[PATCH] D18649: [clang-tidy] cppcoreguidelines-interfaces-global-init

[Clement Courbet via cfe-commits]

courbet added a comment.

In http://reviews.llvm.org/D18649#391001, @alexfh wrote:

> In http://reviews.llvm.org/D18649#390862, @courbet wrote:
>
> > In http://reviews.llvm.org/D18649#389363, @alexfh wrote:
> >
> > > Thank you for working on the new clang-tidy check!
> > >
> > > We usually recommend authors to run their checks on a large code base to ensure it doesn't crash and doesn't generate obvious false positives. It would be nice, if you could provide a quick summary of such a run (total number of hits, number of what seems to be a false positive in a sample of ~100).
> >
> >
> > The tool generated 20k positives on our codebase. On a sample of 100, there are:
> >
> > - 8 instances of the same exact code structure that's just wrong: const string var = FLAGS_some_flag + "some_sufix";
> > - 8 false positives.
> > - 84 possible issues. (interestingly 6 of these are from premature use of variations of "extern char* empty_string;"
> >
> >   The false positives fall into 3 categories:
> > - 3 variations of: ``` extern int i; static const int* pi = &i;  // diag ```
>
>
> Should we warn at all when only an address of a global variable is used?


We could, but it would allow stuff like:

  extern const int i;
  const char c = *(const char*)&i;

No strong opinion here.

> 

> 

> > // Then pi is dereferenced later, once i is intialized. 

> 

> >  Public example of this: https://github.com/python-git/python/blob/py3k/Objects/dictobject.c#L2027

> 

> > 

> 

> > 2. 3 variations of: ``` // .h class A { static const int i = 42; }; // .cc int A::i;  // diag ```

> 

> 

> Looks like we have all information to fix this kind of a false positive.

> 

> > 

> 

> > 

> 

> > 3. 2 variations of: ``` // .h class A { static int i; static int j; }; // .cc int A::i = 0; int A::j = i;  // diag ```

> 

> 

> ditto


It turns out (2) and (3) are just variations of the same - (2) was just:

  // .h
  class A {
    static const int i = 0;
    static const int j = i;
  };
  // .cc
  int A::i;
  int A::j;

Which has *exactly* the same semantics as (3).

I could not find a way to go from a decl to its (possible) definition. I was thinking of matching all global definitions that are not declarations and then making sure that the current matches are not referring to one of those definitions (in syntactic order because of [3.6.2.3]). Does this  sound like a reasonable idea ?


http://reviews.llvm.org/D18649