[PATCH] D51949: [WIP][clang-tidy] initial ideas to isolate variable declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+  auto Diag =
+      diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+      << static_cast<unsigned int>(
----------------
lebedev.ri wrote:
> kbobyrev wrote:
> > JonasToth wrote:
> > > kbobyrev wrote:
> > > > How about `multiple declarations within a single statement hurts readability`?
> > > s/hurts/reduces/? hurts sound a bit weird i think.
> > > 
> > > Lebedev wanted the number of decls in the diagnostic, would you include it or rather now?
> > "decreases" is also fine. "hurts" is probably too strong, I agree.
> > 
> > Up to you. Personally, I don't see any value in having the diagnostic message saying "hey, you have 2 declarations within one statement, that's really bad!" or "hey, you have 5 declarations within one statement..." - in both cases the point is that there are *multiple* declarations. I also don't think it would make debugging easier because you also check the formatting, so you already imply that the correct number of declarations was detected.
> > 
> > I'm interested to know what @lebedev.ri thinks.
> > I'm interested to know what @lebedev.ri thinks.
> 
> "This translation unit has an error. Can not continue" is also a diagnostic message.
> Why are we not ok with that one, and want compiler to be a bit more specific?
> 
> Similarly here, why just point out that this code is bad as per the check,
> without giving a little bit more info, that you already have?
> "This translation unit has an error. Can not continue" is also a diagnostic message.
>Why are we not ok with that one, and want compiler to be a bit more specific?
>
> Similarly here, why just point out that this code is bad as per the check, without giving a little bit more info, that you already have?

More information doesn't always equate into more understanding, especially when that information causes a distraction. For instance, you could argue that the type of the declared variables is also information we already have, but what purpose would it serve to tell it to the user?

Can you give an example where the specific number of declarations involved would help you to correct the diagnostic? I can't come up with one, so it feels to me like having the count is more of a distraction; especially given that there's no configurable threshold for "now you have too many declarations". I'd feel differently if there was a config option, because then the count is truly useful to know.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949