[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h:144
+    // includes the full path.
+    if (SM.getFilename(IL).contains("UnifiedSource")) {
+      StringRef Name = SM.getFilename(SL);
----------------
NoQ wrote:
> NoQ wrote:
> > probinson wrote:
> > > george.karpenkov wrote:
> > > > NoQ wrote:
> > > > > george.karpenkov wrote:
> > > > > > Is this `if` really necessary? This logic has too much overfitting, and it seems that if someone decides to include `.cc` files, we should analyze them in any case, right? We also would prefer to not stop working if webkit decides on using a different naming for those.
> > > > > This is indeed an act of overfitting. But also there are very few reasons to include a non-header file, and all of them are pretty exotic. I'm not sure we want to analyze these files in all cases. So i want to play safe until we gather more data.
> > > > I would still say that just analyzing included c++ files is a lesser evil.
> > > Agreed.  WebKit is not the only project that does this kind of thing.
> > The question is, are all such projects in fact interested in having the analyzer analyze the respective code?
> > 
> > For instance, if the included code is autogenerated by an external tool, users are probably not interested in finding bugs in such code because they will be unable to fix them.
> > 
> > Are you interested in providing a list of examples of projects that do this kind of thing, explain why are they doing it, and whether they will be interested in having warnings in included files?
> Note that we're not talking about warnings that are *emitted* in the included file. We're talking about warnings that *originate* within the included file (and as such most likely stay within the included file and its includes). This makes a difference because we are doing inter-procedural analysis. I.e., if we find that passing a null pointer in the main file into a header function causes a null dereference in the header function, the report that originates within the main file will be emitted in the header with auxiliary path notes in the main file.
> 
> So if a small chunk of code is included but the main file still contains reasonable stuff, we'll be able to find most bugs in the included code as it is being used from the rest of the file. The problem only arises when all code is included.
Game teams are known to use "unity" builds, developing their code in separate files then having a master .cpp that will `#include` everything in a given library/component.  This separates source into reasonable sized and logically coherent units, but the unity build improves build time for these large projects.  If a team is using static analysis, they will want it on the entire compilation.


https://reviews.llvm.org/D45839