Skip to main content
fixed typo
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

The other review makes most of the points I would have, so I'll just add twothree more points.

Don't ignore the return value of mktemp

For security reasons, mktemp will refuse to create a file if the name it has chosen already exists. This is to avoid security flaws. You will probably be saved because /tmp/ should always have the sticky bit set, preventing users other than the owner of the file from deleting it. Still, I'd recommend a construction like this instead:

TMPFILE=$(mktemp /tmp/example.XXXXXXXXXX) || exit 1

Another option would be to omit the output file name and just allow the compiler to create a.out in the local directory, bypassing the issue entirely as well as shortening your script.

Use the result directly instead of testing $?

The code includes this:

g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE
if [ $? -eq 0 ]; then

But this could be made a little more direct:

if g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE ; then

Or perhaps better, see the next suggestion.

Consider using functions

I'd suggest it may be useful to create functions, such as run and compile to make the script a little easier to understand. For example:

compile() {
    WARNING_FLAGS="-Wuninitialized -Wmaybe-uninitialized"
    g++ "$1" -std=c++17 $WARNING_FLAGS -O3 -o "$2"
}

if compile "$FILE" "$TMPFILE"; then
 

The other review makes most of the points I would have, so I'll just add two more points.

Don't ignore the return value of mktemp

For security reasons, mktemp will refuse to create a file if the name it has chosen already exists. This is to avoid security flaws. You will probably be saved because /tmp/ should always have the sticky bit set, preventing users other than the owner of the file from deleting it. Still, I'd recommend a construction like this instead:

TMPFILE=$(mktemp /tmp/example.XXXXXXXXXX) || exit 1

Another option would be to omit the output file name and just allow the compiler to create a.out in the local directory, bypassing the issue entirely as well as shortening your script.

Use the result directly instead of testing $?

The code includes this:

g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE
if [ $? -eq 0 ]; then

But this could be made a little more direct:

if g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE ; then

Or perhaps better, see the next suggestion.

Consider using functions

I'd suggest it may be useful to create functions, such as run and compile to make the script a little easier to understand. For example:

compile() {
    WARNING_FLAGS="-Wuninitialized -Wmaybe-uninitialized"
    g++ "$1" -std=c++17 $WARNING_FLAGS -O3 -o "$2"
}

if compile "$FILE" "$TMPFILE"; then
 

The other review makes most of the points I would have, so I'll just add three more points.

Don't ignore the return value of mktemp

For security reasons, mktemp will refuse to create a file if the name it has chosen already exists. This is to avoid security flaws. You will probably be saved because /tmp/ should always have the sticky bit set, preventing users other than the owner of the file from deleting it. Still, I'd recommend a construction like this instead:

TMPFILE=$(mktemp /tmp/example.XXXXXXXXXX) || exit 1

Another option would be to omit the output file name and just allow the compiler to create a.out in the local directory, bypassing the issue entirely as well as shortening your script.

Use the result directly instead of testing $?

The code includes this:

g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE
if [ $? -eq 0 ]; then

But this could be made a little more direct:

if g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE ; then

Or perhaps better, see the next suggestion.

Consider using functions

I'd suggest it may be useful to create functions, such as run and compile to make the script a little easier to understand. For example:

compile() {
    WARNING_FLAGS="-Wuninitialized -Wmaybe-uninitialized"
    g++ "$1" -std=c++17 $WARNING_FLAGS -O3 -o "$2"
}

if compile "$FILE" "$TMPFILE"; then
 
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

The other review makes most of the points I would have, so I'll just add two more points.

Don't ignore the return value of mktemp

For security reasons, mktemp will refuse to create a file if the name it has chosen already exists. This is to avoid security flaws. You will probably be saved because /tmp/ should always have the sticky bit set, preventing users other than the owner of the file from deleting it. Still, I'd recommend a construction like this instead:

TMPFILE=$(mktemp /tmp/example.XXXXXXXXXX) || exit 1

Another option would be to omit the output file name and just allow the compiler to create a.out in the local directory, bypassing the issue entirely as well as shortening your script.

Use the result directly instead of testing $?

The code includes this:

g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE
if [ $? -eq 0 ]; then

But this could be made a little more direct:

if g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE ; then

Or perhaps better, see the next suggestion.

Consider using functions

I'd suggest it may be useful to create functions, such as run and compile to make the script a little easier to understand. For example:

compile() {
    WARNING_FLAGS="-Wuninitialized -Wmaybe-uninitialized"
    g++ "$1" -std=c++17 $WARNING_FLAGS -O3 -o "$2"
}

if compile "$FILE" "$TMPFILE"; then