[PATCH] D13731: [RFC][Analyzer] Supporting function attributes in .model files.

[pierre gousseau via cfe-commits]

pgousseau added inline comments.

================
Comment at: lib/Analysis/BodyFarm.h:43
@@ +42,3 @@
+  /// \brief Merge the attributes found in model files.
+  /// Note, this adds all attributes found in the model file without any sanity
+  /// checks.
----------------
zaks.anna wrote:
> Why do we not have sanity checks? What happens when there is a conflict?
I am worried that any custom sanity check I put would end up confusing the "faux-attributes" user ... I have just noticed that Sema has some merging capabilities, I will have a look, see if it can be reused. Thanks!

================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:505
@@ +504,3 @@
+// AST visitor used to merge model attributes.
+class WalkAST : public DataRecursiveASTVisitor<WalkAST> {
+  AnalysisDeclContextManager &Mgr;
----------------
zaks.anna wrote:
> This name is too generic.
Will rename it thanks!

================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:541
@@ +540,3 @@
+    // If "faux-attributes=true" is given, merge model's attributes.
+    AnalysisDeclContextManager &ADCMgr = Mgr->getAnalysisDeclContextManager();
+    if (ADCMgr.mergeModelAttributes()) {
----------------
zaks.anna wrote:
> Should we walk the entire AST here only to get the info from the few functions in the farm? 
> 
> The AnalysisConsumer visitor tries to make sure the whole AST is not serialized, which is very expensive when dealing with PCH files. (You an find comments about it if you search for "PCH".)
I see, yes this can be optimized thanks!

================
Comment at: lib/StaticAnalyzer/Frontend/ModelConsumer.cpp:37
@@ +36,3 @@
+    // We are interested in definitions and declarations.
+    FunctionDecl *func = llvm::dyn_cast<FunctionDecl>(*I);
+    if (func) {
----------------
zaks.anna wrote:
> Why func lost constness here?
No reason, will fix thanks!

================
Comment at: lib/StaticAnalyzer/Frontend/ModelInjector.cpp:49
@@ -43,3 +48,3 @@
   // about file name index? Mangled names may not be suitable for that either.
-  if (Bodies.count(D->getName()) != 0)
+  if (Bodies.count(D->getName()) != 0 || Decls.count(D->getName()) != 0)
     return;
----------------
zaks.anna wrote:
> Does ModelInjector::onBodySynthesis return immediately if the model has been parsed but the Decl is not in the map?
> 
> If that is not the case, wouldn't it be very expensive to call onBodySynthesis on every decl, most of which are not modeled? If I understand correctly, we would try to parse the model file for every such decl. 
> 
Yes, there might be cases where the model file's Decl might not match the model file filename, causing re-parsing. Will fix thanks!

================
Comment at: test/Analysis/model-attributes.cpp:2
@@ +1,3 @@
+// This is testing the 'faux-attributes' analyzer option.
+// The declaration of 'modelFileHasAttributes' found in modelFileHasAttributes.model has 'nonnull' attributes on the 2nd and 3rd parameter.
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -analyze -analyzer-checker=core -analyzer-config faux-attributes=true,model-path=%S/Inputs/Models %s -verify
----------------
zaks.anna wrote:
> 80 col?
Will fix!


http://reviews.llvm.org/D13731