[PATCH] D102669: [analyzer][ctu] Fix wrong 'multiple definitions' errors caused by space characters in lookup names when parsing the ctu index file

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:154-172
+  // Find the length delimiter.
+  const size_t LengthDelimiter = LineRef.find(':');
+  if (StringRef::npos == LengthDelimiter)
+    return false;
+
+  // Parse the length of LookupName as USRLength.
+  size_t USRLength = 0;
----------------
OikawaKirie wrote:
> steakhal wrote:
> > 
> The source of lookup name of the function being imported is function `CrossTranslationUnitContext::getLookupName`. Keeping the length in the mapping can avoid parsing the lookup name during importing.
Okay; you can copy the original StringRef to have that. But by consuming it on one path makes the code much more readable.



================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:183
+/// Check whether the input lookup name is in format "<USR-Length>:<USR>" by
+/// appending a space charactor and parse with parseCrossTUIndexItem.
+bool isCTUIndexKeyValid(StringRef N) {
----------------
charactor  -> character


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:184-188
+bool isCTUIndexKeyValid(StringRef N) {
+  std::string FN = N.str() + ' ';
+  StringRef LN, FP;
+  return parseCrossTUIndexItem(FN, LN, FP) && N == LN && FP.empty();
+}
----------------
You should probably use more elaborative names, I wouldn't know what this does if I hadn't reviewed this patch.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:463
     bool DisplayCTUProgress) {
+  // Make sure the input lookup name is in format "<USR-Length>:<USR>".
+  assert(isCTUIndexKeyValid(FunctionName) &&
----------------
The assertion speaks for itself. It rarely needs additional documentation.


================
Comment at: clang/test/Analysis/Inputs/ctu-lookup-name-with-space.cpp:7-8
+    // CHECK-NOT: multiple definitions are found for the same key in index
+    f([](char) -> int { return 42; });
+    f([](char) -> bool { return true; });
+  }
----------------
I would rather put these into the `importee()`


================
Comment at: clang/test/Analysis/Inputs/ctu-lookup-name-with-space.cpp:13
+int importee(int X) {
+  return 1 / X;
+}
----------------
Why do you need to have a div by zero warning?


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

https://reviews.llvm.org/D102669