[PATCH] D52794: [analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Please add a test case where a bug path goes through a macro definition and this macro is undefed at the end of the translation unit.



================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:667
+
+//===----------------------------------------------------------------------===//
+// Declarations of helper functions and data structures for expanding macros.
----------------
Maybe it would be worth to move these helpers to a separate file and add some documentation why we actually simulate the preprocessor rather than doing something else.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:724
+  TokenPrinter Printer(OS, PP);
+  return { getExpandedMacroImpl(Printer, MacroLoc, PP), OS.str() };
+}
----------------
Maybe could directly return ExpansionInfo?


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:734
+  MacroNameAndInfo Info = getMacroNameAndInfo(SM.getExpansionLoc(MacroLoc), PP);
+  std::string MacroName = std::move(Info.Name);
+  const MacroInfo *MI = Info.MI;
----------------
I think this local might be redundant.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:753
+    // macro.
+    if (const MacroInfo *MI = PP.getMacroInfo(II)) {
+      getExpandedMacroImpl(Printer, T.getLocation(), PP);
----------------
This might not be completely correct. We are using the preprocessor state after processing the whole translation unit rather than the preprocessor state at the expansion location. This might not cause any problems unless there is a collision between macro names and non-macro names. Also, undefs, redefs might cause troubles.


================
Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:796
+  const MacroInfo *MI = PP.getMacroInfo(II);
+  assert(MI && "This IdentifierInfo should refer to a macro!");
+
----------------
Could this assertion fail due to undefs in the code?


https://reviews.llvm.org/D52794