firolino added inline comments.
================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:34
+ // a tag declaration (e.g. struct, class etc.):
+ // class A { } Object1, Object2; <-- won't be matched
+ Finder->addMatcher(
----------------
aaron.ballman wrote:
> firolino wrote:
> > aaron.ballman wrote:
> > > firolino wrote:
> > > > aaron.ballman wrote:
> > > > > firolino wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > firolino wrote:
> > > > > > > > firolino wrote:
> > > > > > > > > firolino wrote:
> > > > > > > > > > firolino wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > firolino wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > Why do we not want to match this?
> > > > > > > > > > > > > If we decide, whether we transform
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > class A {
> > > > > > > > > > > > > } Object1, Object2;
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > to
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > class A {
> > > > > > > > > > > > > } Object1,
> > > > > > > > > > > > > Object2;
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > or
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > class A {
> > > > > > > > > > > > > }
> > > > > > > > > > > > > Object1,
> > > > > > > > > > > > > Object2;
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > I might consider adding support for it. Moreover,
> > > > > > > > > > > > > this kind of definition is usually seen globally and
> > > > > > > > > > > > > I don't know how to handle globals yet. See
> > > > > > > > > > > > > http://lists.llvm.org/pipermail/cfe-dev/2015-November/046262.html
> > > > > > > > > > > > I think this should be handled. It can be handled in
> > > > > > > > > > > > either of the forms you show, or by saying:
> > > > > > > > > > > > ```
> > > > > > > > > > > > A Object1;
> > > > > > > > > > > > A Object2;
> > > > > > > > > > > > ```
> > > > > > > > > > > > If all of these turn out to be a problem, we can still
> > > > > > > > > > > > diagnose without providing a fixit.
> > > > > > > > > > > >
> > > > > > > > > > > > As for globals in general, they can be handled in a
> > > > > > > > > > > > separate patch once we figure out the declaration
> > > > > > > > > > > > grouping.
> > > > > > > > > > > OK. I will try to split the object definition from the
> > > > > > > > > > > class definition, as you have suggested. Thus, I can kick
> > > > > > > > > > > out the tagDecl-matcher as well. If there is no easy way
> > > > > > > > > > > to do this, it will be reported anyway but without a
> > > > > > > > > > > fixit.
> > > > > > > > > > >
> > > > > > > > > > > Note for me: Update documentation!
> > > > > > > > > > What about
> > > > > > > > > > ```
> > > > > > > > > > struct S {
> > > > > > > > > > } S1;
> > > > > > > > > > ```
> > > > > > > > > > I would like to report this too, since two names are being
> > > > > > > > > > declared here. `S` and `S1`. What do you think?
> > > > > > > > > ```
> > > > > > > > > struct {
> > > > > > > > > } nn1, nn2;
> > > > > > > > > ```
> > > > > > > > > Shall we ignore anonymous definitions?
> > > > > > > > To be more precise: Warn and provide a fixit for `struct S {}
> > > > > > > > S1`. Only warn for `struct {} nn1, nn2`.
> > > > > > > > What about
> > > > > > > >
> > > > > > > > ```
> > > > > > > > struct S {
> > > > > > > > } S1;
> > > > > > > > ```
> > > > > > > > I would like to report this too, since two names are being
> > > > > > > > declared here. S and S1. What do you think?
> > > > > > >
> > > > > > > I don't think that this should be diagnosed. For one, this is a
> > > > > > > relatively common pattern (arguably more common than `struct S {
> > > > > > > } S1, S2;`), but also, it names two very distinct entities (a
> > > > > > > type and a variable).
> > > > > > >
> > > > > > > > ```
> > > > > > > > struct {
> > > > > > > > } nn1, nn2;
> > > > > > > >```
> > > > > > > > Shall we ignore anonymous definitions?
> > > > > > >
> > > > > > > Yes, we basically have to.
> > > > > > >
> > > > > > Transforming
> > > > > > ```
> > > > > > typedef struct X { int t; } X, Y;
> > > > > > ```
> > > > > > to
> > > > > > ```
> > > > > > typedef struct X { int t; };
> > > > > > typedef X X;
> > > > > > typedef X Y;
> > > > > > ```
> > > > > > will be valid, but looks odd.
> > > > > I am on the fence about this transformation -- it does not declare
> > > > > new objects, but instead declares new types. A very common pattern in
> > > > > some libraries (such as the Win32 SDK) is to declare:
> > > > > ```
> > > > > typedef struct Blah {
> > > > >
> > > > > } Blah, *PBlah, **PPBlah;
> > > > > ```
> > > > > Because of that pattern, diagnosing the above typedef is likely to be
> > > > > very chatty in some projects, and I don't know that the fixit
> > > > > provides much real benefit for types.
> > > > >
> > > > > At least for the CERT DCL04-C version of this rule, that code should
> > > > > not be flagged. Do you think any of the variations of this check
> > > > > should flag it?
> > > > Maybe we should reconsider where to split and where not. The original
> > > > idea was to minimize confusion on code like
> > > > ```
> > > > int* a,b;
> > > > ```
> > > > whereas
> > > > ```
> > > > struct S {} *s1, s2;
> > > > ```
> > > > seems not so confusing as above. However,
> > > > ```
> > > > struct S {}* s1, s2;
> > > > ```
> > > > looks dangerous again.
> > > >
> > > > Even following code looks unfamiliar to me.
> > > > ```
> > > > typedef struct Blah {} Blah, *PBlah;
> > > > ```
> > > >
> > > > It really depends on the beholder. So, how about letting this check
> > > > split **everything** and provide options for maximum flexibility? So
> > > > specific rulesets or the user may configure it as they wish. We could
> > > > add for example split-after-tag-def, split-tag-typedef in addition to
> > > > cppcore and cert and use a default setting (split-after-tag-def=true,
> > > > split-tag-typedef=false).
> > > I've typically found that users go with whatever the default options are,
> > > rather than specify a slew of options, and if the default options produce
> > > an onerous check, they simply disable the entire check. I'd rather not
> > > push the design off to the users unless there's really no better
> > > alternative.
> > >
> > > I think that the only "dangerous" operations this sort of check is meant
> > > to catch is when the declaration group uses different specifiers for the
> > > declarator or when there's an initializer on one declarator but none of
> > > the rest. I think the other operations are more likely to be a
> > > consistency thing than a bug-prone thing. e.g.,
> > > ```
> > > // Differing specifiers
> > > int i, j, k; // Not at all likely to be mistaken code.
> > > int i, *j, &k; // Not really likely to be mistaken code.
> > > int *i, j; // More likely to be mistaken code.
> > > int i, *j; // Not really likely to be mistaken code.
> > > typedef int *one, two, three; // More likely to be mistaken code.
> > > typedef int one, *two, **three; // Not really likely to be mistaken code.
> > >
> > > // Initialization
> > > int i = 10, j = 10; // Not at all likely to be mistaken code.
> > > int i = 10, j; // Not really likely to be mistaken code.
> > > int i, j = 10; // More likely to be mistaken code.
> > > ```
> > > I think declarations that declare both types and variables should ignore
> > > the type name and focus only on the variable name following the same
> > > heuristics. e.g.,
> > > ```
> > > struct S {} s; // Not at all likely to be mistaken code.
> > > struct S {} s, *t; // Not really likely to be mistaken code.
> > > struct S {} *s, t; // More likely to be mistaken code.
> > > ```
> > > I could imagine an option like `FlagPossiblyDangerous` or something else;
> > > if the flag is set, the check diagnoses based on some heuristic of likely
> > > dangerous operations and if the flag is not set, the check flags all
> > > declarators in the same declaration.
> > >
> > > What do you think?
> > We are judging from our perspective what is "likely dangerous" and this
> > might not agree with the users view. This is why I don't wanna force any
> > limitations based on our decisions. This check might not be used only to
> > reduce "readability error" but even for a simple style guide enforcement
> > that wants one variable declaration per line.
> >
> > I know that `int i, j, k;` is not bad at all or likely to be mistaken, but
> > style guides from [[ https://www.openssl.org/policies/codingstyle.txt |
> > openssl]] or the [[
> > https://www.kernel.org/doc/Documentation/process/coding-style.rst | linux
> > kernel ]] are enforcing one declaration per line.
> >
> > > I've typically found that users go with whatever the default options are
> > So, why not simple use options and set the default whatever we think is
> > right. We are not loosing anything with this way, just getting more
> > flexible. Anyone else can reset the options to their needs.
> >
> > I would think of following options:
> > `Style = Default, CppCore, Cert`
> > `SplitTagDef = True, False`
> > `SplitTagTypeDef = True, False`
> >
> > and with default:
> > ```
> > Style = Default # splits anything, regardless of tokens and initializers
> > SplitTagDef = True
> > SplitTagTypeDef = False
> > ```
> >
> > The check will be registered under cert with `Style = Cert` and `CppCore`
> > for cppcore respectively.
> >
> > And since it is topic related, there are still two things on my todo list:
> > - attributes
> > - members
> >
> > ```
> > struct alignas(8) S {} s1, s2;
> > alignas(8) char a, b;
> > [[maybe_unused]] bool x, y;
> >
> > struct S {
> > int a, b;
> > };
> > ```
> > Should we include this as well, or leave it for future work?
> >
> > So, why not simple use options and set the default whatever we think is
> > right. We are not loosing anything with this way, just getting more
> > flexible. Anyone else can reset the options to their needs.
>
> I think we are describing the same solution, but perhaps from different
> angles. I am proposing that we have this checker flag everything, but that we
> have a sensible set of default options that reduce the number of diagnostics
> issued for existing code bases. I fear that if the default behavior of this
> check is to point out *every* declaration group with more than one name being
> declared, the output will be way too chatty and so the entire check is
> disabled. I think that if we had a default behavior that only pointed out the
> suspicious declarations, that would reduce the "noise" by default. One way to
> test this would be to run the check over some large code bases of various
> style guides to see how many results there are. I would recommend running it
> over the LLVM and Clang code bases (which do not have a restriction on
> declarations) and OpenSSL (which does have this restriction) and report back
> the diagnostics you get from each (as well as totals). Feel free to pick
> other/additional code bases to test again.
>
> > Should we include this as well, or leave it for future work?
>
> It's up to you, but I think it's reasonable to leave for future work.
> Attributes are likely to be somewhat harder than members in many regards due
> to where attributes can be written. I suspect that members can be handled
> like variable declarations in general.
OK, I get what you mean. Btw, thanks for taking your time! This review gets
longer and longer :)
https://reviews.llvm.org/D27621
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
