lebedev.ri added a comment.

I have looked through tests, and it is possible that i just missed it, but does 
it test/handle the cases like:

  struct S {
    int X, Y, Z;  // <- should be diagnosed.
  }



================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+  auto Diag =
+      diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+      << static_cast<unsigned int>(
----------------
JonasToth wrote:
> aaron.ballman wrote:
> > JonasToth wrote:
> > > kbobyrev wrote:
> > > > 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.
> > > I am personally against having the number in the diagnostic as well, it 
> > > would only add value if the declarations are expanded from a macro.
> > > 
> > > @aaron.ballman Configuration of this check would be intersting but i 
> > > would rather postpone that and have a basic working check first. Given 
> > > that this aims to be utility-like to evaluate `const-correctness` and/or 
> > > to be usable with other checks doing type transformations.
> > Yeah, I wasn't suggesting a threshold config option for this patch so much 
> > as pointing out why I'm opposed to putting the count in the diagnostic.
> I used a different wording without the variable count. Adding configuration, 
> more advanced diagnostic can be done in a follow up or in a different check, 
> e.g. `readability-function-size`, as it does counting (might not be the 
> perfect fit though)
I, too, wasn't suggesting a config option.
It really is about not having more than one variable per declaration, not more 
than N variables per declaration.
I would personally prefer to see the number (since it complains about the 
count), but it will be possible to live without it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to