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

[Anna Zaks via cfe-commits]

zaks.anna 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.
----------------
Why do we not have sanity checks? What happens when there is a conflict?

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

================
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()) {
----------------
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".)

================
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) {
----------------
Why func lost constness here?

================
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;
----------------
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. 


================
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
----------------
80 col?


http://reviews.llvm.org/D13731