steakhal added a comment.
Please mark comments 'done' if they are done.
================
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:
> > 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.
================
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; });
+ }
----------------
OikawaKirie wrote:
> steakhal wrote:
> > I would rather put these into the `importee()`
> The lambda exprs will not be included in the CTU index file if they are
> declared in a normal function.
I see.
================
Comment at: clang/test/Analysis/Inputs/ctu-lookup-name-with-space.cpp:13
+int importee(int X) {
+ return 1 / X;
+}
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102669/new/
https://reviews.llvm.org/D102669
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
