[PATCH] D133009: [libclang] Fix conversion from `StringRef` to `CXString`

Thu Sep 1 05:29:06 PDT 2022

egorzhdan added inline comments.

================
Comment at: clang/test/Index/comment-lots-of-unknown-commands.c:3

+// XFAIL: *
+
----------------
aaronpuchert wrote:
> egorzhdan wrote:
> > gribozavr2 wrote:
> > > egorzhdan wrote:
> > > > This test was never properly passing. Because of the bug in string conversion, the printed comments contained the entire source file and not just the comments' text, which was enough to cause `// CHECK`-s in the test to succeed.
> > > > ```
> > > > // CHECK:         (CXComment_InlineCommand CommandName=[tel] RenderNormal HasTrailingNewline)
> > > > // CHECK:         (CXComment_InlineCommand CommandName=[n] RenderNormal HasTrailingNewline))
> > > > // CHECK:       (CXComment_VerbatimLine Text=[\n at Lo\n at il\n at tle\n at axt\n at ba\n at ust\n at ac\n at tpe\n at tpl\n at ctG\n at ru\n at m\n at tG\n at it\n at rh\n at G\n at rpc\n at el\n at er\n at w\n at eo\n at tx\n at oo\n at dD\n at dD\n*/\nvoid f();\n\n// CHECK:  CommentAST=[\n// CHECK:    (CXComment_FullComment\n// CHECK:       (CXComment_Paragraph\n// CHECK:      ...
> > > > ```
> > > Please update the test to pass then. Here's the diff:
> > > 
> > > ```
> > > diff --git a/clang/test/Index/comment-lots-of-unknown-commands.c b/clang/test/Index/comment-lots-of-unknown-commands.c
> > > index 41a03d394488..e1adcc150b1e 100644
> > > --- a/clang/test/Index/comment-lots-of-unknown-commands.c
> > > +++ b/clang/test/Index/comment-lots-of-unknown-commands.c
> > > @@ -1,6 +1,5 @@
> > >  // RUN: c-index-test -test-load-source-reparse 1 local %s | FileCheck %s
> > > 
> > > -// XFAIL: *
> > > 
> > >  // See PR 21254. We had too few bits to encode command IDs so if you created
> > >  // enough of them the ID codes would wrap around. This test creates commands up
> > > @@ -183,7 +182,7 @@ void f();
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[ei] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[oun] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[ou] RenderNormal HasTrailingNewline)
> > > -// CHECK:         (CXComment_InlineCommand CommandName=[nl] RenderNormal HasTrailingNewline)
> > > +// CHECK:         (CXComment_InlineCommand CommandName=[n] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[ien] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[fr] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[en] RenderNormal HasTrailingNewline)
> > > @@ -204,7 +203,7 @@ void f();
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[fro] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[ast] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[ae] RenderNormal HasTrailingNewline)
> > > -// CHECK:         (CXComment_InlineCommand CommandName=[nN] RenderNormal HasTrailingNewline)
> > > +// CHECK:         (CXComment_InlineCommand CommandName=[n] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[pc] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[tae] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[ws] RenderNormal HasTrailingNewline)
> > > @@ -268,10 +267,8 @@ void f();
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[an] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[de] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[tel] RenderNormal HasTrailingNewline)
> > > -// CHECK:         (CXComment_InlineCommand CommandName=[nd] RenderNormal HasTrailingNewline)
> > > -// CHECK:         (CXComment_InlineCommand CommandName=[dic] RenderNormal HasTrailingNewline)
> > > +// CHECK:         (CXComment_InlineCommand CommandName=[n] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[Lo] RenderNormal HasTrailingNewline)
> > > -// CHECK:         (CXComment_InlineCommand CommandName=[il] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[tle] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[axt] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[ba] RenderNormal HasTrailingNewline)
> > > @@ -283,7 +280,6 @@ void f();
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[ru] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[m] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[tG] RenderNormal HasTrailingNewline)
> > > -// CHECK:         (CXComment_InlineCommand CommandName=[it] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[rh] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[G] RenderNormal HasTrailingNewline)
> > >  // CHECK:         (CXComment_InlineCommand CommandName=[rpc] RenderNormal HasTrailingNewline)
> > > ```
> > > 
> > This doesn't look like the correct output though. I think the `CommandName` fields in the output should match the `@`-tags in the input (e.g. `@s` -> `(CXComment_InlineCommand CommandName=[s] RenderNormal HasTrailingNewline)`).
> > 
> > This looks like a bug in the logic this test is aiming to verify, not a mistake in the test itself. I unfortunately don't have enough context to fix the actual issue here, so I disabled the test instead. I'll also file an GitHub issue for this.
> They don't always match, because typo correction will change commands that are close enough to an existing command. That's why e.g. `@nl` shows up as `@n` in the AST. I remember that I had to change this test in D111190 because introducing the new commands made the typo correction kick in for some of these.
> 
> The proper fix for the test (see the comment below) should be to change the commands here to differ enough from existing commands again. The corrections to `@n` are likely caused by D125429, while `@dic` → `@dir`, `@il`/`@it` → `@if` come from D111190.
> 
> I don't think we're going to gain additional commands, but we might also make this "future-proof" by simply using long-enough sequences of random letters, let's say 4 to have a reasonable number.
Thanks @aaronpuchert! I didn't realize this is caused by typo correction. I'll put up a patch to re-enable the test.

================
Comment at: clang/tools/libclang/CXString.cpp:81-82
 CXString createRef(StringRef String) {
+  if (!String.data())
+    return createNull();
+
----------------
aaronpuchert wrote:
> Does this actually happen or are we just doing this to be safe?
It's possible to get an empty StringRef with a nullptr data here. I initially skipped this `if` and got a test failure because of it: instead of returning a null CXString, this method was returning an empty CXString.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133009