aaron.ballman 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(
----------------
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?
https://reviews.llvm.org/D27621
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
