Removing commented dead code without removing the legitimate comments

Question

This question is the real question asked on StackOverflow. I'm here to review my answer and see how can I optimize it.

---

Here is the answer text:

This is a basic approach, but it proposes a proof of concept of what might be done. I do it using Bash along with the usage of the GCC -fsyntax-only option.

Here is the bash script:

#!/bin/bash
while IFS='' read -r line || [[ -n "$line" ]]; do
    LINE=`echo $line | grep -oP "(?<=//).*"`
    if [[ -n "$LINE" ]]; then
            echo $LINE | gcc -fsyntax-only -xc -
            if [[ $? -eq 0 ]]; then
                   sed -i "/$LINE/d" ./$1
            fi
    fi
done < "$1"

The approach I followed here was reading each line from the code file. Then, greping the text after the // delimiter (if exists) with the regex (?<=//).* and passing that to the gcc -fsyntax-only command to check whether it's a correct C/C++ statement or not. Notice that I've used the argument -xc - to pass the input to GCC from stdin (see my answer here to understand more). An important note, the c in -xc - specifies the language, which is C in this case, if you want it to be C++ you shall change it to -xc++.

Then, if GCC was able to successfully parse the statement (i.e., it's a legitimate C/C++ statement), I directly remove it using sed -i from the file passed.

---

Running it on your example (but after removing <- commented code from the third line to make it a legitimate statement):

// Those parameters control foo and bar... <- valid comment
int t = 5;
// int t = 10;
int k = 2*t;

Output (in the same file):

// Those parameters control foo and bar... <- valid comment
int t = 5;
int k = 2*t;

(if you want to add your modifications in a different file, just remove the -i from sed -i)

The script can be called just like: ./script.sh file.cpp, it may show several GCC errors while these are the correct

Answer 1

• echo | grep is unwarranted. bash understands regular expressions ("$line" =~ regex), and can do simple substitutions: line=${line#[[:space:]]*\/\/} removes leading whitespaces , followed by the comment, just what we are after.

• Replacing the file while reading it looks suspiciously. I recommend to have a destination file, and copy valid lines (and don't copy undesired ones). A perk benefit is that forking sed is not needed anymore.

A side note: the script makes a false positive in cases like

    // Notice that
    // some_valid_c_code;
    // doesn't work, because etc

The part of the comment would be recognized as a dead code, and the output will be

    // Notice that
    // doesn't work, because etc

Answer 2

Look for corner cases

This command is fragile, there are several ways in which it can malfunction:

sed -i "/$LINE/d" ./$1

For example:

• If the dead code contains /, it will break the sed command, because / within /.../d must be escaped.

• It doesn't target accurately the line to remove. It removes all lines that match $LINE. If there are lines in the file that are similar enough to a dead code that appears somewhere else, it will be removed too.

Both of these problems can be fixed by tracking the line numbers that should be deleted, and then using those with the d command of sed, instead of pattern matching.

---

The pattern "(?<=//).*" used by the grep is not strict enough, and may incorrectly match lines that are not dead code, for example:

int x = 1;  // some comment
char * s = "foo // bar";

Double-quote variables used in command line arguments

How many bugs can you spot here?

while IFS='' read -r line || [[ -n "$line" ]]; do
    somecmd ./$1
done < "$1"

I see at least:

• It doesn't handle absolute paths correctly. When $1 is an absolute path, then ./$1 and "$1" are likely different files, except in the lucky case when the working directory is /.

• ./$1 is not properly quoted, so if $1 contains spaces or shell meta-characters, the command will fail.

The solution is simple: quote properly and use the same path consistently somecmd "$1".

In addition, it's usually a good idea to assign command line arguments to variables with descriptive names at the top of a script, and then refer to it by that name, instead of have $1 scattered at multiple places in the script.

Use the exit code of commands directly in conditional statements

Instead of this:

somecmd
if [[ $? -eq 0 ]]; then
       ...
fi

You can write:

if somecmd; then
       ...
fi

Simpler and quite natural!

Avoid sed -i ... somefile in a loop

Repeatedly rewriting the content of a file in a loop, looks dangerous.

Use here-strings

Usually echo "..." | somecommand can be rewritten as somecommand <<< "...", using here-strings, and saving an echo and a pipe.

Then depending on somecommand, better options may be available, such as using [[ ... =~ ... ]] for pattern matching instead of grep (as @vnp mentioned), or running grep on a larger outer scope (as demonstrated in the previous point).

Alternative implementation

Consider this alternative implementation that fixes the above issues and bad practices.

#!/usr/bin/env bash

input=$1

sed_commands=()
line_num=1
while IFS= read -r line || [[ "$line" ]]; do
    if [[ "$line" =~ ^[[:space:]]+// ]]; then
        if gcc -fsyntax-only -xc - <<< "$line"; then
            sed_commands+=(-e "${line_num}d")
        fi
    fi
    ((line_num++))
done < "$input"

sed "${sed_commands[@]}" -i "$input"

The weakness of this alternative is that if there are enough dead code lines in the input, then the maximum argument count limit of the shell may be reached in the final sed command. When that becomes a realistic issue, it can be optimized to handle that.