[PATCH] D60974: Clang IFSO driver action.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi marked 6 inline comments as done.
plotfi added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:626
   HelpText<"Use the LLVM representation for assembler and object files">;
+def emit_ifso : Flag<["-"], "emit-interface-stubs">, Flags<[CC1Option]>, Group<Action_Group>,
+  HelpText<"Generate Inteface Stub Files.">;
----------------
compnerd wrote:
> I thought that we were going to add `experimental` to this for the time being?
Oh, I was specifying experimental in the -interface-stubs-version=experimental-ifo-elf-v1. Do you want the main flag to also be -emit-interface-stubs-experimental??


================
Comment at: clang/include/clang/Driver/Types.def:91
 TYPE("ast",                      AST,          INVALID,         "ast",   "u")
+TYPE("ifs",                      IFS,          INVALID,         "ifs",   "u")
 TYPE("pcm",                      ModuleFile,   INVALID,         "pcm",   "u")
----------------
compnerd wrote:
> What about `ifo` instead of `ifs`?
I went with ifs because ifo implies the analog of a .o file and ifso implies the analog of a .so file. I want the tbe/ifs text files to just be thought of as something a little different. Like symbol listings that another tool can assemble into whatever format.  


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1649
+    case OPT_emit_ifso:
+      Opts.ProgramAction = frontend::GenerateIfsoTbeExpV1;
+      if (Args.hasArg(OPT_ifso_version_EQ))
----------------
compnerd wrote:
> Can we make this required to be specified instead of defaulting please?  I think that it is more clear.
I think that makes sense. 


================
Comment at: clang/lib/Frontend/FrontendActions.cpp:179
+  };
+  using MangledSymbols = std::map<const NamedDecl *, MangledSymbol>;
+
----------------
compnerd wrote:
> Hmm, I wonder if `DenseSet` is more appropriate here.  Is there some reason that I am overlooking for supporting duplicate entries?  In fact, since the key is a pointer, you could even do a `DenseMap`.
Can we do data-structure improvements post landing the first version? 


================
Comment at: clang/lib/Frontend/FrontendActions.cpp:214
+      ND->dump();
+    }
+    return true;
----------------
compnerd wrote:
> debugging left over?
Ah yeah I need to replace this with an llvm_unreachable or something. 


================
Comment at: clang/lib/Frontend/FrontendActions.cpp:297
+        sema.LateTemplateParser(sema.OpaqueParser, LPT);
+        HandleNamedDecl(FD, Symbols, (FromTU | IsLate));
+      }
----------------
compnerd wrote:
> Typo of `||`?  The field is boolean not a mask.
It is a bitmask. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974