I’ve stumbled on some uses of if False/if True in the stack, which apparently were debugging statements that got left in on commit, or blocks of code that were intentionally left in for future reference. Here’s an example:
There are 116 instances of if False: in the stack, and 23 if True:s. Quite a few are in tests. There’s also a couple dozen #if 0 and #if 1 in the .h and .cc files, many of which appear in the SWIG wrapped files. Apparently those were put in by SWIG, though its not clear?
I was told that this was often used because the LSST coding standards do not allow commented out code. I couldn’t find a reference to that in the coding standards: is there an explicit statement about this? To my mind, if there is a good reason to not just delete the line, commenting it out with an attached comment saying why, e.g. “# this should be faster, but it gives the wrong result”, seems much better than wrapping it in “if False”: the commented line doesn’t disrupt one’s flow when reading whereas you have to parse the conditional, it adds an indent level, and editors don’t give you color syntax hints about which branch gets executed.
Similarly, “#if 0” in C++: why not just comment out that block? Some editors (e.g. SublimeText) will color the now-ignored block, but not all editors do. I know /* */ can break if there’s another /* */ block in the middle, but this should not be used for big blocks of code anyway.
Is there a global #define LSST_DEBUG that one can use in C++, and/or something similar in python? That at least would make it slightly more clear about whether a given block of code exists just for debugging purposes.
If the coding standards already say something about this, please point me to them, otherwise I think the standards probably should include some discussion of this.
I think @parejkoj makes an excellent point about readability. It is far too easy to mistake code that is commented out by “if False” or “#if 0”, especially Python code, where the code will always appear to be runnable. Some editors gray out C++ code hidden by “#if 0”, though by no means all do.
My recollection is that the if form is there for two reasons:
To make sure the code is compiled or checked for syntax errors
To make the code easily runnable
However, I lean towards John’s view that readability is more important. If code is to be left around as “I tried this and it didn’t work” or “this could be developed into something great” then it can be commented out and is still easy to enable if desired.
I’m not keen on a variable such as LSST_DEBUG, due to the danger of it leaking into files where it is not wanted.
I agree that disabled code should usually be taken out. The exception: if there’s a good reason to leave it in there should be comments explaining why the disabled code is there. If there’s a date, Jira case number, or other condition where it could or should be re-enabled then that should be included in the comment.
I don’t have any strong opinion about how it’s disabled (# (or //), “if False”, “#if 0” etc) (except in C++ the /* … */ form can be problematic and should probably be avoided, IMO.
I use if False/#if 0 for code that is correct but not currently needed, e.g. complex debugging output, alternative safe-but-slow implementations, things I might want to jump to in the debugger. In my experience these can be extremely valuable when you’re running into problems with new data and want to know what’s going on. You could put in a comment with a reference to the changeset that removed them, but realistically l prefer to leave them in.
In C and C++ you have to prefix each line with // as /* … */ doesn’t nest. #if 0 is cleaner if it’s something you want to turn back on (and sometimes you do).