compnerd added inline comments.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1690
+ std::make_pair(frontend::GenerateInterfaceTBEExpV1, false));
+ if (!ProgramActionPair.second)
+ Diags.Report(diag::err_drv_invalid_value)
----------------
I think you could've used a `llvm::Optional`.
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:21
+ StringRef InFile;
+ StringRef Format;
+ std::set<std::string> ParsedTemplates;
----------------
An enumeration may be nicer.
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:39
+ bool WriteNamedDecl(const NamedDecl *ND, MangledSymbols &Symbols, int RDO) {
+ if (!(RDO & FromTU))
+ return true;
----------------
I tend to prefer `if (RDO & ~FromTU)`
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:56
+ auto isVisible = [this](const NamedDecl *ND) -> bool {
+ if (ND->getVisibility() != DefaultVisibility) {
+ if (ND->hasAttr<VisibilityAttr>())
----------------
Why not `if (ND->getVisibility() == DefaultVisibility) return true;`?
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:65
+ };
+ auto doBail = [this, isVisible](const NamedDecl *ND) -> bool {
+ if (!isVisible(ND))
----------------
I think that using something like `ignoreDecl` is better than `doBail`.
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:68
+ return true;
+ if (const VarDecl *VD = dyn_cast<VarDecl>(ND))
+ if ((VD->getStorageClass() == StorageClass::SC_Extern) ||
----------------
A newline before this would be nice.
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:72
+ VD->getParentFunctionOrMethod() == nullptr))
+ return true;
+ if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
----------------
If it is a `VarDecl`, it cannot be a `FunctionDecl`. I think that this can be
simplified a bit as:
```
if (const auto *VD = dyn_cast<VarDecl>(VD))
return VD->getStorageClass() == StorageClass::SC_Extern ||
VD->getStorageClass() == StorageClass::SC_Static ||
VD->getParentFunctionOrMethod() == nullptr;
```
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:73
+ return true;
+ if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
+ if (FD->isInlined() && !isa<CXXMethodDecl>(FD) &&
----------------
Newline before this would be nice. I think that using `auto` here for the type
is fine as you spelt out the type in the `dyn_cast`.
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:77
+ return true;
+ if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD)) {
+ if (const auto *RC = dyn_cast<CXXRecordDecl>(MD->getParent())) {
----------------
I think that flipping this around would be nicer.
```
if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
...
}
return FD->isInlined() && !Instance.getLangOpts().GNUInline;
```
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:79
+ if (const auto *RC = dyn_cast<CXXRecordDecl>(MD->getParent())) {
+ if (const auto *CTD = dyn_cast<ClassTemplateDecl>(RC->getParent()))
+ return true;
----------------
`CTD` is not used, please use `isa` and avoid the unused variable warning.
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:118
+ return MangledName;
+ };
+
----------------
Can we compress `getMangledName` and `getMangledNames` into `getMangledNames`
and always return the vector? This avoids the duplication and simplifies the
symbol recording later as well.
================
Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:150
+ if (IsRDOLate)
+ llvm_unreachable("Generating Interface Stubs is not supported with "
+ "delayed template parsing.");
----------------
You can probably hoist this to L129, and avoid the variable definition and the
check on L131.
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
