NoQ added a comment.

In https://reviews.llvm.org/D54438#1296832, @Szelethus wrote:

> - Some checkers do not register themselves in all cases[1], which should 
> probably be handled at a higher level, preferably in `Checkers.td`. Warnings 
> could be emitted when a checker is explicitly enabled but will not be 
> registered. [1]




In https://reviews.llvm.org/D54438#1296832, @Szelethus wrote:

>   void ento::registerObjCDeallocChecker(CheckerManager &Mgr) {
>     const LangOptions &LangOpts = Mgr.getLangOpts();
>     // These checker only makes sense under MRR.
>     if (LangOpts.getGC() == LangOptions::GCOnly || LangOpts.ObjCAutoRefCount)
>       return;
>  
>     Mgr.registerChecker<ObjCDeallocChecker>();
>   }
>


The can of worms here is as follows.

It is clear that not all checkers are applicable to all languages or all 
combinations of compiler flags. In this case, significantly different semantics 
are observed under different Objective-C memory management policies and the 
checker isn't designed to work correctly for anything but the "manual" memory 
management policy. The way the code is now written, it is impossible to 
register the checker for translation units that are compiled with incompatible 
flags.

This should not cause a warnings because it should be fine to compile different 
translation units with different flags within the same project, while the 
decision to enable the checker explicitly is done once per project.

Now, normally these decisions are handled by the Clang Driver - the gcc 
compatibility wrapper for clang that converts usual gcc-compatible run-lines 
into `-cc1` run-lines. Eg., the `unix` package is only enabled on UNIX targets 
and the `osx` package is only enabled on macOS/iOS targets and the `core` 
package is enabled on all targets and `security.insecureAPI` is disabled on PS4 
//because that's what the Driver automatically appends to the -cc1 run-line//. 
Even though `scan-build` does not call the analyzer through the Driver, it 
invokes a `-###` run-line to obtain flags first, so it still indirectly 
consults the Driver.

But the problem with the Driver is that nobody knows about this layer of 
decision-making (unlike the obvious `Checkers.td`) and therefore it's kinda 
messy to have all checkers hardcoded separately within the Driver. It kinda 
makes more sense to split the checkers into packages and let the Driver choose 
which packages are enabled.

But the problem with packages is that they are very hard to change because they 
are the UI that external GUIs rely upon. And right now we don't yet have 
separate packages for Objective-C MRR/ARC/GC modes (btw, GC needs to be 
removed).

And even if we were to go in that direction, the amount of packages would 
explode exponentially with the number of different independent flags we need to 
track. If only we had hashtags, that would have been a great direction to go.

So, generally, i suggest not to jump onto this as long as the analyzer as a 
whole works more or less correctly. Restricting functionality for the purpose 
of avoiding mistakes should be done with extreme care because, well, it 
restricts functionality. Before restricting funcitonality, we need to be more 
or less certain that nobody will ever need such functionality or that we will 
be able to provide a safe replacement when necessary without also introducing 
too much complexity (aka bugs). Which is why i recommend not to bother too much 
about it. There are much more terrible bugs all over the place, no need to 
struggle that hard to prevent these small bugs.

Another approach to the original problem would be to replace the language 
options check in registerBlahBlahChecker() with a set of language options 
checks in every checker callback. It's annoying to write and clutters the 
checker but it preserves all the functionality without messing with the 
registration process. So if you simply want to move these checks out of the way 
of your work, this is probably the way to go.


Repository:
  rC Clang

https://reviews.llvm.org/D54438



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to