rupprecht added a reviewer: rupprecht.
rupprecht added a comment.
Can you upload this patch with context? Either use arc or upload w/ -U99999
I seem to have a lot of comments, but they're all nits -- my major concern
being the `llvm_unreachable`s should be errors instead (i.e. should be
triggered in release builds).
Since this is clearly labeled as experimental, I think you should feel free to
commit if you can get another lgtm (@compnerd?)
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3614-3616
+ Args.hasArg(options::OPT_iterface_stub_version_EQ)
+ ? Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)
+ : "")
----------------
`Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)` should already
default to returning the empty string if unset (note the default param)
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1678-1680
+ Args.hasArg(OPT_iterface_stub_version_EQ)
+ ? Args.getLastArgValue(OPT_iterface_stub_version_EQ)
+ : "")
----------------
(same here)
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1688
+ if (!ProgramActionPair.second)
+ llvm_unreachable("Must specify a valid interface stub format.");
+ Opts.ProgramAction = ProgramActionPair.first;
----------------
I think this is very much reachable if someone provides
`--interface-stub-version=x`
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:20-21
+ CompilerInstance &Instance;
+ StringRef InFile = "";
+ StringRef Format = "";
+ std::set<std::string> ParsedTemplates;
----------------
nit: these default initializations are redundant
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:26
+ struct MangledSymbol {
+ std::string ParentName = "";
+ uint8_t Type;
----------------
ditto
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:39-41
+ if (!(RDO & FromTU))
+ return true;
+ if (Symbols.find(ND) != Symbols.end())
----------------
Can you add a comment why these two are excluded?
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:41
+ return true;
+ if (Symbols.find(ND) != Symbols.end())
+ return true;
----------------
llvm::is_contained(Symbols, ND)
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:44-47
+ if (isa<FieldDecl>(ND))
+ return true;
+ if (isa<ParmVarDecl>(ND))
+ return true;
----------------
This would be terser (and more readable) combined; `if (isa<FieldDecl>(ND) ||
isa<ParamValDecl>(ND))`
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:95
+ index::CodegenNameGenerator CGNameGen(ND->getASTContext());
+ auto MangledNames = CGNameGen.getAllManglings(ND);
+ if (MangledNames.size() == 1)
----------------
auto -> std::vector<std::string>, unless the return type is obvious to anyone
that commits here (i.e. not me)
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:107
+ index::CodegenNameGenerator CGNameGen(ND->getASTContext());
+ auto MangledName = CGNameGen.getName(ND);
+ auto MangledNames = CGNameGen.getAllManglings(ND);
----------------
auto -> std::string
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:206-207
+ // catch anything that's not a VarDecl or Template/FunctionDecl.
+ llvm_unreachable("clang -emit-iterface-stubs: Expected a function or "
+ "function template decl.");
+ return false;
----------------
This should be an error, not llvm_unreachable (which I think gets filtered out
in release builds)
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:242
+ MangledSymbols Symbols;
+ auto OS = Instance.createDefaultOutputFile(/*Binary=*/false, InFile,
"ifo");
+ if (!OS)
----------------
elsewhere it looks like this was changed to "ifs"?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60974/new/
https://reviews.llvm.org/D60974
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
