[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud added a comment.

Thanks Adrian for the fast response.
See some answers below.

I will update the code and upload a new patch soon.



================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:154
+    StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  // Only care about "include" directives.
+  if (!IncludeTok.is(tok::identifier))
----------------
aprantl wrote:
> 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?
I will remove the switch and update the LastHashLoc unconditionally.
I did not check Objective-C debug info, but I assume it will work fine once I change these lines.


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:88
+  // Invalid location represents line zero.
+  return SourceLocation();
+}
----------------
aprantl wrote:
> I think the entire function can be replaced with
> ```
> if ((Status == MainFileScope) ||
>     (Status == CommandLineScopeIncludes && CommandIncludeFiles))
>   return Loc;
> return SourceLocation();
> ```
Sure, will do.
The reason I have a switch, because I had more cases during the implementation that I get rid of.


================
Comment at: lib/CodeGen/MacroPPCallbacks.cpp:100
+  bool CreateMacroFile = false;
+  bool PopScope = false;
+  SourceLocation LineLoc = getCorrectLocation(LastHashLoc);
----------------
aprantl wrote:
> 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.
Will try restructure.
The way I implemented this is by having stages (parsing levels).
The control flow is to identify the current stage and decide how to continue from there.

I will see what I can do to make the code much clear.


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

Do you think any of the above would be better choice?


https://reviews.llvm.org/D16135