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:
> > 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.
I think what causes the misunderstanding is the definition of //consume// in 
the context of `StringRef`.
```lang=C++
const StringRef Copy = Line;
Line.consumeInteger(...); // Line advances forward by the number of characters 
that were parsed as an integral value.
// Copy still refers to the unmodified, original characters.
// I can use it however I want.

// `Line` is a suffix of `Copy`, and the `.end()` should be the same, only 
`.begin()` should differ.
```

I hope that caused the miscommunication.

---
> 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.
Yeah, sure!


================
Comment at: clang/test/Analysis/Inputs/ctu-lookup-name-with-space.cpp:12-14
+int importee(int X) {
+  return 1 / X;
+}
----------------
Also fixup the return type in the declaration within the main TU. Also add the 
`// expected-no-diagnostics` comment to the primary TU.


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

Reply via email to