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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev 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>(
----------------
aaron.ballman wrote:
> 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.
Oh, but that's different: "This translation unit has an error. Can not continue" does not provide enough information for users to fix the issue, pointing out that there are *multiple* declarations per statement is definitely enough.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949