[PATCH] D16135: Macro Debug Info support in Clang

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

Thanks! Couple more inline comments.



================
Comment at: lib/CodeGen/CGDebugInfo.h:415
 
+  /// Get macro debug info.
+  llvm::DIMacro *CreateMacro(llvm::DIMacroFile *Parent, unsigned MType,
----------------
It would be good to be a bit more descriptive here. If the comment just repeats the name of the function it is not worth keeping,
/// Create debug info for a macro (definition/use/?).


================
Comment at: lib/CodeGen/CGDebugInfo.h:420
+
+  /// Get temporary macro file debug info.
+  llvm::DIMacroFile *CreateTempMacroFile(llvm::DIMacroFile *Parent,
----------------
E.g.,
/// Create debug info for a file referenced by an #include directive.


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:44
+    if (MI.isGNUVarargs())
+      Name << "..."; // #define foo(x...)
+
----------------
aaboud wrote:
> aprantl wrote:
> > LLVM style prefers comments to be full sentences and above the code.
> As I said above, this code is taken from "lib\Frontend\PrintPreprocessedOutput.cpp"
> Also, I ran clang-format on it, and it did not suggest to change this line.
> 
> But, I do not mind fixing this, if it is the preferred comment style.
I think it would be a good idea to clean this up. Feel free to either do this now or in a separate commit.


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:154
+    StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  // Only care about "include" directives.
+  if (!IncludeTok.is(tok::identifier))
----------------
aaboud wrote:
> aprantl wrote:
> > #include as opposed to #import? If so, why?
> I am not familiar with "#import" keyword.
> 1. Is it handled differently by clang that #include?
> 2. Do we need to generate macro debug info for it?
> 
> The reason I have this switch below, is that I am not sure if we call this "InclusionDirective" callback for identifiers that are not "#include".
> 
> However, I see no harm of updating the LastHashLoc every time we enter this function. The value will not be used unless it is set just before calling "FileChanged"callback.
#import comes from Objective-C. It is equivalent to #include + #pragma once.
I'm curious whether this comment means that it isn't handled?


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:88
+  // Invalid location represents line zero.
+  return SourceLocation();
+}
----------------
I think the entire function can be replaced with
```
if ((Status == MainFileScope) ||
    (Status == CommandLineScopeIncludes && CommandIncludeFiles))
  return Loc;
return SourceLocation();
```


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:100
+  bool CreateMacroFile = false;
+  bool PopScope = false;
+  SourceLocation LineLoc = getCorrectLocation(LastHashLoc);
----------------
Could you try splitting this function up into two functions?

MacroPPCallbacks::FileEntered()
MacroPPCallbacks::FileExited()

and the common code factored out into a helper? The control flow here seems unnecessarily complex with all the flags.


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:177
+  llvm::raw_string_ostream Value(ValueBuffer);
+  calculatMacroDefinition(*Id, *MD->getMacroInfo(), PP, Name, Value);
+  Gen->getCGDebugInfo()->CreateMacro(getCurrentScope(),
----------------
calculat*e*MacroDefinition
(is there a better word than calculate?)


https://reviews.llvm.org/D16135