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>( ---------------- lebedev.ri wrote: > 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. Yes, I understand the point, and it might even make sense to refactor that check later on. ATM this is a workhorse for the const-correctness check refactorings, as they need isolated variable declarations ;) 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
