4 ms·

Nice article with a lot of depth. While disable comments are something to pay close attention to in a code review, I disagree that their inclusion makes static

Nice article with a lot of depth. While disable comments are something to pay close attention to in a code review, I disagree that their inclusion makes static analysis tools worse. In fact, I would argue not including them makes static analysis tools worse. As an example, one clang-tidy rule we use is the explicit-constructor rule [1]. In C++, single argument constructors allow implicit conversions by default, which is almost never what you want since it hides type errors. This rule is very useful since it forces explicit constructors to be used. However, in some (rare) cases you *do* want implicit constructors. In that case, you need a way of disabling this rule. The author provides two alternatives to disable comments for this scenario: * Globally disable the rule - Undesirable, now a useful rule must be removed because of 1-2 locations where it is not wanted * Locally disable the rule using a global config file - Essentially the same as disable comments, but worse. Disable comments are useful since they are localized in the code and hence more likely to remain up to date. They can contain explanations for why they are required in a specific location, and their inclusion is obvious which makes it easy to detect and check during a code review. In an ideal world we would not need them, but sadly we do not live in an ideal world. [1] https://clang.llvm.org/extra/clang-tidy/checks/google-explicit-constructor.html https://clang.llvm.org/extra/clang-tidy/checks/google-explic...

scatters 5y ago
Since C++20, you can write `explicit(false)` to indicate that a single argument constructor is intentionally implicit. Similarly, the language adds the [[fallthrough]] statement attribute to indicate intentional fallthrough between switch case labels. As languages improve, we should need disable comments less and less.
- quietbritishjim 5y ago
  While explicit(false) does seem like a good way to indicate that, it seems like it's just a happy accident that it exists. The intended purpose of explicit(xyz) is that xyz is a compile-time constant that depends on the template parameters of the current (template) class. A literal true or false isn't needed by the language (just include explicit or don't), but the language would be more complex if they were disallowed so it's easier to allow them. So in general, improvements to languages don't necessarily remove the need for comments over time. (On the other hand, more radical improvements might remove needs for warning comments by removing the original problem. In this case, in a language where constructors were explicit by default and you had to opt in to implicit, you wouldn't need the warning to exist in the first place.)
  - bluGill 5y ago
    > In this case, in a language where constructors were explicit by default and you had to opt in to implicit, you wouldn't need the warning to exist in the first place A warning that authors of new languages should account for. However most of us have millions of lines of working code, and experience in our current language. As such changing to your new better language isn't trivial. That is before we get into how old languages have had a lot of effort into tools and optimizations that new ones don't have [yet?] thus making the old one better in some potentially important dimensions.
l0b0 5y ago
A little discipline will go a long way here: - If the annotation is thought to be necessary because of a bug in the tool, add a link to the bug next to the disable comment. This makes it easy to get rid of the comment when being alerted that the bug is fixed. - If you happen to disagree with the tool in a specific instance, and there's no way to override just that case, mention why in the commit message. - Check for disable comments at peer review.
- quietbritishjim 5y ago
  Isn't it useful for the tool to warn about something that is often incorrect but sometimes intentional? The parent's example of implicit constructors is a good one, and a sibling comment mentioned fallthrough as another example. None of your suggestions help with that: - It's not a bug in the tool (because the warning is often useful). - A mention in a commit message doesn't help when you run the tool in future and get false alarms (oh I'll just read all the historic commit messages to see if any say these should be ignored). - If you find such a comment at peer review then that doesn't give you anything actionable to do if the comment is being used correctly in one of these casees. The solution in these cases is not "a little discipline" but simply to use the disabling comment and accept that this is reasonable.
  - bluGill 5y ago
    > Isn't it useful for the tool to warn about something that is often incorrect but sometimes intentional? Only if the total number of warnings across your entire codebase is tiny - 10 is the [arbitrary] number I like to use. That is 10 warnings, it doesn't matter if you are writing 5 lines of hello world or a 100 million lines of a self driving car. When the number of ignored warnings get above that number people start to ignore the tool and then it becomes useless.
    - quietbritishjim 5y ago
      Your comment drives the discussion in a rather circular fashion: your objection is addressed by code comments that selectively disable warnings at specific locations in code, which is the subject of the original article. So long as you make careful use of those then it doesn't matter if you have 10,000 of them in your 100 million lines of code; you still only get one (visible) warning if you make a change that triggers it again, which is below your 10 warnings threshold.
EdwardDiego 5y ago
Aye - on my current (Java) project, my general rule of thumb is if I'm suppressing a compiler/IDE warning or codestyle rule, I comment as to why. E.g., am I suppressing raw-types warning because I couldn't be arsed getting the type signatures right? Or because some library we're using only accepts a `List<Future>` when every other usage of `Future` in the method is a `Future<T>`, so you need to use a raw `List` at some point? I try to suppress as few warnings as possible, so only suppress warnings that are due to third party limitations like the above, or because I think that the warning is, in this instance, not helpful if obeyed. But once again, I'll always comment to explain this _why_
moomin 5y ago
Agree completely. Whilst it’s true you can use disable comments to make code worse, removing them is a technological solution to a social problem. Static analysis helps with implementing coding standards and good practice, but you still need to actually have the users care.

scatters 5y ago

Since C++20, you can write `explicit(false)` to indicate that a single argument constructor is intentionally implicit. Similarly, the language adds the [[fallthrough]] statement attribute to indicate intentional fallthrough between switch case labels. As languages improve, we should need disable comments less and less.

quietbritishjim 5y ago
While explicit(false) does seem like a good way to indicate that, it seems like it's just a happy accident that it exists. The intended purpose of explicit(xyz) is that xyz is a compile-time constant that depends on the template parameters of the current (template) class. A literal true or false isn't needed by the language (just include explicit or don't), but the language would be more complex if they were disallowed so it's easier to allow them. So in general, improvements to languages don't necessarily remove the need for comments over time. (On the other hand, more radical improvements might remove needs for warning comments by removing the original problem. In this case, in a language where constructors were explicit by default and you had to opt in to implicit, you wouldn't need the warning to exist in the first place.)
- bluGill 5y ago
  > In this case, in a language where constructors were explicit by default and you had to opt in to implicit, you wouldn't need the warning to exist in the first place A warning that authors of new languages should account for. However most of us have millions of lines of working code, and experience in our current language. As such changing to your new better language isn't trivial. That is before we get into how old languages have had a lot of effort into tools and optimizations that new ones don't have [yet?] thus making the old one better in some potentially important dimensions.

l0b0 5y ago

A little discipline will go a long way here: - If the annotation is thought to be necessary because of a bug in the tool, add a link to the bug next to the disable comment. This makes it easy to get rid of the comment when being alerted that the bug is fixed. - If you happen to disagree with the tool in a specific instance, and there's no way to override just that case, mention why in the commit message. - Check for disable comments at peer review.

quietbritishjim 5y ago
Isn't it useful for the tool to warn about something that is often incorrect but sometimes intentional? The parent's example of implicit constructors is a good one, and a sibling comment mentioned fallthrough as another example. None of your suggestions help with that: - It's not a bug in the tool (because the warning is often useful). - A mention in a commit message doesn't help when you run the tool in future and get false alarms (oh I'll just read all the historic commit messages to see if any say these should be ignored). - If you find such a comment at peer review then that doesn't give you anything actionable to do if the comment is being used correctly in one of these casees. The solution in these cases is not "a little discipline" but simply to use the disabling comment and accept that this is reasonable.
- bluGill 5y ago
  > Isn't it useful for the tool to warn about something that is often incorrect but sometimes intentional? Only if the total number of warnings across your entire codebase is tiny - 10 is the [arbitrary] number I like to use. That is 10 warnings, it doesn't matter if you are writing 5 lines of hello world or a 100 million lines of a self driving car. When the number of ignored warnings get above that number people start to ignore the tool and then it becomes useless.
  - quietbritishjim 5y ago
    Your comment drives the discussion in a rather circular fashion: your objection is addressed by code comments that selectively disable warnings at specific locations in code, which is the subject of the original article. So long as you make careful use of those then it doesn't matter if you have 10,000 of them in your 100 million lines of code; you still only get one (visible) warning if you make a change that triggers it again, which is below your 10 warnings threshold.

EdwardDiego 5y ago

Aye - on my current (Java) project, my general rule of thumb is if I'm suppressing a compiler/IDE warning or codestyle rule, I comment as to why. E.g., am I suppressing raw-types warning because I couldn't be arsed getting the type signatures right? Or because some library we're using only accepts a `List<Future>` when every other usage of `Future` in the method is a `Future<T>`, so you need to use a raw `List` at some point? I try to suppress as few warnings as possible, so only suppress warnings that are due to third party limitations like the above, or because I think that the warning is, in this instance, not helpful if obeyed. But once again, I'll always comment to explain this _why_

moomin 5y ago

Agree completely. Whilst it’s true you can use disable comments to make code worse, removing them is a technological solution to a social problem. Static analysis helps with implementing coding standards and good practice, but you still need to actually have the users care.