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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
