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