alexfh added a comment. In http://reviews.llvm.org/D18649#391112, @courbet wrote:
> 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. `VarDecl` is also `Redeclarable<VarDecl>`, which has various ways to iterate over all related declarations. > 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
