[PATCH] D60974: Clang IFSO driver action.

[Saleem Abdulrasool via Phabricator via cfe-commits]

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