JonasToth 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>( ---------------- 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. 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
