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

[Ella Ma via Phabricator via cfe-commits]

OikawaKirie 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;
----------------
steakhal wrote:
> OikawaKirie wrote:
> > steakhal wrote:
> > > 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.
> > > 
> > The `getAsInterger` call can also check whether the content before the first colon is an integer. Therefore, a sub-string operation is required here.
> I don't doubt that your proposed way of doing this works and is efficient.
> What I say is that I think there is room for improvement in the non-functional aspects, in the readability. However, it's not really a blocking issue, more of a personal taste.
I know what you are considering, it is clearer and more readable by consuming the length, then the USR. However, to correctly separate the USR and file path, the length of `USR-Length` is also required, which makes it impossible to just *consume* the length at the beginning.

Another way of solving this problem is to re-create the string with the USR-Length and the USR after parsing, but I think it is not a good solution.

BTW, is it necessary to assert the `USR-Length` to be greater than zero? I think it is more reasonable to report *invalid format* rather than assert the value, as it can be provided by the user.


================
Comment at: clang/test/Analysis/Inputs/ctu-lookup-name-with-space.cpp:13
+int importee(int X) {
+  return 1 / X;
+}
----------------
steakhal wrote:
> OikawaKirie wrote:
> > steakhal wrote:
> > > Why do you need to have a div by zero warning?
> > I am not sure whether we should test if an external function can be correctly imported. Hence, I wrote a div-by-zero bug here. A call to function `clang_analyzer_warnIfReached` is also OK here.
> > 
> > As the imported lambda expr will not be called, I think I can only test whether CTU works via another normal function.
> AFAIK importing a function and import-related stuff are orthogonal to actually emitting bug reports produced by the analyzer. That being said, if the `importee()` would have an empty body, the observable behavior would remain the same. And this is what I propose now.
Sorry, but I am not quite clear about your suggestions on this function.


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

https://reviews.llvm.org/D102669