[PATCH] D55134: [CTU] Add triple/lang mismatch handling

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Please find my comments inline.

> Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be an issue there?

That's a good question. In Samsung, CTU hasn't been tested on ObjC code. Upstream CTU supports only FunctionDecls as well so its ObjC status is questionable..



================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:31
 
+namespace llvm {
+// Same as Triple's equality operator, but we check a field only if that is
----------------
Why don't we place it into the anon namespace just below?


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:35
+bool hasEqualKnownFields(const Triple &Lhs, const Triple &Rhs) {
+  return ((Lhs.getArch() != Triple::UnknownArch &&
+           Rhs.getArch() != Triple::UnknownArch)
----------------
This has to be split, probably with early returns. Example:
```if (Lhs.getArch() != Triple::UnknownArch && Rhs.getArch() != Triple::UnknownArch &&
              Lhs.getArch() != Rhs.getArch()
  return false;
...```


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:47
+              : true) &&
+         ((Lhs.getOS() != Triple::UnknownOS && Rhs.getOS() != Triple::UnknownOS)
+              ? Lhs.getOS() == Rhs.getOS()
----------------
We can use `Triple::isOSUnknown()` instead.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:59
+}
+}
+
----------------
Szelethus wrote:
> `// end of namespace llvm`
`// end namespace llvm` is much more common.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:203
 
+  const auto& TripleTo = Context.getTargetInfo().getTriple();
+  const auto& TripleFrom = Unit->getASTContext().getTargetInfo().getTriple();
----------------
Reference char should stick to the variable, not to the type.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:211
+    // TODO pass the SourceLocation of the CallExpression for more precise
+    // diagnostics
+    Context.getDiagnostics().Report(diag::err_ctu_incompat_triple)
----------------
Comments should end with a dot. Same below.


================
Comment at: test/Analysis/ctu-unknown-parts-in-triples.cpp:8
+// RUN: cp %S/Inputs/externalFnMap.txt %t/ctudir/externalFnMap.txt
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection  -analyzer-config experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%t/ctudir -verify %s
+
----------------
Two spaces after `ExprInspection`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55134