Skip to content

[clang-doc] Add regression test for test comments in macros #132510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 6, 2025
Merged

[clang-doc] Add regression test for test comments in macros #132510

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a6a1916
Fixes #59819. The underlying problem was fixed in https://reviews.llv…
ZhongUncle Mar 22, 2025
b15a9d0
Fixes #59819. The underlying problem was fixed in https://reviews.llv…
ZhongUncle Mar 22, 2025
2b6556e
Fixes #59819. The underlying problem was fixed in https://reviews.llv…
ZhongUncle Mar 22, 2025
875938c
Fixes #59819. The underlying problem was fixed in https://reviews.llv…
ZhongUncle Mar 25, 2025
7bad199
Update clang-tools-extra/test/clang-doc/comments-in-macros.cpp
ZhongUncle Apr 2, 2025
9e9fdf6
modified: comments-in-macros.cpp
ZhongUncle Apr 19, 2025
f9cd05d
modified: comments-in-macros.cpp
ZhongUncle May 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions clang-tools-extra/test/clang-doc/comments-in-macros.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Regression test for https://github.com/llvm/llvm-project/issues/59819

// RUN: rm -rf %t && mkdir -p %t
// RUN: clang-doc --format=md --doxygen --output=%t --executor=standalone %s
// RUN: FileCheck %s < %t/GlobalNamespace/MyClass.md --check-prefix=MD-MYCLASS-LINE
// RUN: FileCheck %s < %t/GlobalNamespace/MyClass.md --check-prefix=MD-MYCLASS

// RUN: clang-doc --format=html --doxygen --output=%t --executor=standalone %s
// RUN: FileCheck %s < %t/GlobalNamespace/MyClass.html --check-prefix=HTML-MYCLASS-LINE
// RUN: FileCheck %s < %t/GlobalNamespace/MyClass.html --check-prefix=HTML-MYCLASS

#define DECLARE_METHODS \
/** \
* @brief Declare a method to calculate the sum of two numbers\
*/ \
int Add(int a, int b) { \
return a + b; \
}

// MD-MYCLASS: ### Add
// MD-MYCLASS: *public int Add(int a, int b)*
// MD-MYCLASS: **brief** Declare a method to calculate the sum of two numbers

// HTML-MYCLASS: <p>public int Add(int a, int b)</p>
// HTML-MYCLASS: <div>brief</div>
// HTML-MYCLASS: <p> Declare a method to calculate the sum of two numbers</p>
Copy link
Contributor

@ilovepi ilovepi Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't clear to me from the test if the defined at... part is matching where you want it. the following checks in the class body should probably have the -NEXT: suffix so we know we're matching the last part of the the Add() defintion w/in the macro.

Copy link
Contributor Author

@ZhongUncle ZhongUncle Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't clear to me from the test if the defined at... part is matching where you want it. the following checks in the class body should probably have the -NEXT: suffix so we know we're matching the last part of the the Add() defintion w/in the macro.

Yeah, I notice its line is in class, rather than line comment in macro.
Do you mean: -NEXT: HTML-MYCLASS: <p>public int Add(int a, int b)</p>

I don't know how to use it and can't find usage in documentation. Can you give me a example of -NEXT:? It is better to give me a documentation or manual.

Copy link
Contributor Author

@ZhongUncle ZhongUncle Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have another problem: Why HTML match is not similar to Markdown?

Because I refer to enum.cpp to write this match. It matchs different parts. So I don't make sense which part must match in HTML.

Markdown match is clearly, HTML seem to ignore something.

Copy link
Contributor

@ilovepi ilovepi Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-next-directive has information on how the -NEXT suffix works.

You use it the following way.

MYCHECK: match first line
MYCHECK-NEXT: match has to be on following line after the previous MYCHECK
MYCHECK: match something (potentially) far away
MYCHECK-NEXT: match has to be on the following line after the previous MYCHECK

What this allows you do do is be certain you're matching the correct part of the output, and that you're not accidentally matching things out of order.

This becomes more obvious w/ things in codegen where you have easy to spot delimiters, but are also likely to have many duplicate lines in your test program (e.g. most load instructions look the same).

So you use this pattern to match the start of a unique sequence w/ MYCHECK (or whatever your prefix is), and then use MYCHECK-NEXT to make sure the following lines match right afterwards, and not anywhere else.

Copy link
Contributor

@ilovepi ilovepi Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have another problem: Why HTML match is not similar to Markdown?

Because I refer to enum.cpp to write this match. It matchs different parts. So I don't make sense which part must match in HTML.

Markdown match is clearly, HTML seem to ignore something.

I'm not sure what you are asking. FileCheck pattern matching will match what it finds in the file that matches. IIRC it will take the last thing it finds that matches, not the first. So your lines may match arbitrary things in the output.

The markdown has a lot of empty lines, which makes writing checks w/ a -NEXT suffix hard to do. HTML is much more structured, so its (generally) more difficult to accidentally match something if you write stricter checks.

Copy link
Contributor Author

@ZhongUncle ZhongUncle Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the explanation, I'll learn about these new LLVM concepts and get familiar with the FileCheck tools, and then improve the code. However, recently I am preparing for the interview for graduate school, so please forgive me for the slow progress.



class MyClass {
public:
// MD-MYCLASS-LINE: *Defined at {{.*}}clang-tools-extra{{[\/]}}test{{[\/]}}clang-doc{{[\/]}}macro.cpp#[[@LINE+2]]*
// HTML-MYCLASS-LINE: <p>Defined at line [[@LINE+1]] of file {{.*}}clang-tools-extra{{[\/]}}test{{[\/]}}clang-doc{{[\/]}}macro.cpp</p>
Copy link
Contributor

@ilovepi ilovepi Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line number seems to not be where you expect in the test. Does this pass for you locally?

Copy link
Contributor Author

@ZhongUncle ZhongUncle Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line number seems to not be where you expect in the test. Does this pass for you locally?

The line number is relative position, they are correct. But in other test of clang-doc, these line below where they are testing.

Not passed may be due to \ in EOL of comment. We talked about it in previous cloesd issue #132360 (comment).

If I add \to each EOL in comment, it will generate some \ in doc generated. So it doesn't passed. You said I can add \ to each EOL, and report it as a new issue.

I have a question: Do we need it to generate completely correct Markdown/HTML codes and pass the test? Or can it have some harmless errors and pass the test?

Copy link
Contributor

@ilovepi ilovepi Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand how the relative line number works in the check line. I'm asking if this test passes when you run the test. So does ninja check-clang-extra-clang-doc pass w/ your changes? I think the line number reported is the line that defines the class, not where you wrote the macro that got expanded.

Do we need it to generate completely correct Markdown/HTML codes and pass the test? Or can it have some harmless errors and pass the test?

Well, its a test, so it needs to be correct w.r.t. the thing you're testing, and shouldn't be broken overall (or at least not broken in a new way).

That said, if there's an issue where the line number is not reported correctly, and that is a new/unrelated issue, then I'm OK if there is a corresponding tracking bug, and we have a TODO in the test referring to that bug.

Copy link
Contributor Author

@ZhongUncle ZhongUncle Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does ninja check-clang-extra-clang-doc pass w/ your changes?

No, it doesn't passed w/ my change. I also test it via llvm-lit llvm-project/clang-tools-extra/test/clang-doc/.

Because below code

#define DECLARE_METHODS                                           \
    /**                                                           \
     * @brief Declare a method to calculate the sum of two numbers\
     */                                                           \
    int Add(int a, int b) {                                       \
        return a + b;                                             \
    }

will generate something like below (there are additional \ appearing):

\    
Declare a method to calculate the sum of two numbers\

rather than correct content generated:

Declare a method to calculate the sum of two numbers

But I test it using correct content generated. It is why not pass test. They don't match, so not pass.

We discussed this rare comment style before, and you suggested that I add a \ to each EOL, because it is example in #59819. But recently, I noticed via the editor highlighting that if you add a \, it will be considered as a character in the comment, rather than indicating the comment as a whole. If you add a \, it will be considered as a character in the comment.

Below image is in vim, you can see color of \ is same as comment (I also test it in VS Code, same highlight):
截屏2025-04-17 14 39 51

So I don't think that it is issues of other feature. The problem is our example.

At beginning, I use this kind of comment

#define DECLARE_METHODS                                           \
    /**                                                           
     * @brief Declare a method to calculate the sum of two numbers
     */                                                           \
    int Add(int a, int b) {                                       \
        return a + b;                                             \
    }

It passed test. So I think: Should I change code like above?

Well, its a test, so it needs to be correct w.r.t. the thing you're testing, and shouldn't be broken overall (or at least not broken in a new way).

Oh, make sense. I will remove this line check in Class. I checked the line numbers here because I saw it written in other tests, so I thought I had to check all the key parts.

Copy link
Contributor

@ilovepi ilovepi Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may be talking past one another.

  1. the test needs to pass before it can land. I won't approve a patch with broken tests.
  2. The test should reflect what's actually going on right now. We may want to add some XFAIL test that has the correct behavior too.
  3. looking at the test output, I don't see anything having to do w/ the \ characters in the macro and the failing test check
/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-clshd-1/llvm-project/github-pull-requests/clang-tools-extra/test/clang-doc/comments-in-macros.cpp:31:21: error: MD-MYCLASS-LINE: expected string not found in input
// MD-MYCLASS-LINE: *Defined at {{.*}}clang-tools-extra{{[\/]}}test{{[\/]}}clang-doc{{[\/]}}macro.cpp#[[@LINE+2]]*
                    ^
<stdin>:1:1: note: scanning from here
# class MyClass
^
<stdin>:1:1: note: with "@LINE+2" equal to "33"
# class MyClass
^
<stdin>:3:95: note: possible intended match here
*Defined at /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-clshd-1/llvm-project/github-pull-requests/clang-tools-extra/test/clang-doc/comments-in-macros.cpp#29*
                                                                                              ^

Input file: <stdin>
Check file: /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-clshd-1/llvm-project/github-pull-requests/clang-tools-extra/test/clang-doc/comments-in-macros.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            1: # class MyClass 
check:31'0     X~~~~~~~~~~~~~~~ error: no match found
check:31'1                      with "@LINE+2" equal to "33"
            2:  
check:31'0     ~
            3: *Defined at /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-clshd-1/llvm-project/github-pull-requests/clang-tools-extra/test/clang-doc/comments-in-macros.cpp#29* 
check:31'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

That output says the check didn't see what you told it to expect. It found a line that looks right, but the line # is wrong (29 instead of 33). I don't see anyting failing due to a stray \ in the output.

You're also asking about changing the macro, but the way you're trying to change it makes it invalid as a macro, so no, you can't do that. If you don't belevie me, dump the preprocessor output, and/or dump the AST w/ clang.

Oh, make sense. I will remove this line check in Class. I checked the line numbers here because I saw it written in other tests, so I thought I had to check all the key parts.

Those are important things to test. You're just not testing it correctly, or more accuratly, you're expectation of the current behavior isn't correct. You have two choices 1) update the test to output the current line number and file an issue that it works in an unexpected way, or 2) fix the implementation in clang-doc so that it gets the correct line number. I'd suggest 1), since I expect fixing the implementation to be quite challenging.

Copy link
Contributor Author

@ZhongUncle ZhongUncle Apr 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I check the code, this error is due to we change the filename macro.cpp to comments-in-macros.cpp. But not change in line number match, you can see it. It doesn't failed via matching wrong line number.

This is a very funny mistake. I didn't find this error because I check code by clang-doc generated code directly, rather than error log. I am so sorry about it.

After I change the filename here, you can see the \ in error log:

/home/zhonguncle/Desktop/llvm-project/clang-tools-extra/test/clang-doc/comments-in-macros.cpp:25:18: error: HTML-MYCLASS: expected string not found in input
// HTML-MYCLASS: <p> Declare a method to calculate the sum of two numbers</p>
                 ^
<stdin>:22:18: note: scanning from here
 <div>brief</div>
                 ^
<stdin>:23:2: note: possible intended match here
 <p> Declare a method to calculate the sum of two numbers\</p>
 ^

Input file: <stdin>
Check file: /home/zhonguncle/Desktop/llvm-project/clang-tools-extra/test/clang-doc/comments-in-macros.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            .
            .
            .
           17:  <p>Defined at line 30 of file /home/zhonguncle/Desktop/llvm-project/clang-tools-extra/test/clang-doc/comments-in-macros.cpp</p> 
           18:  <div> 
           19:  <div> 
           20:  <p>\</p> 
           21:  <div> 
           22:  <div>brief</div> 
check:25'0                      X error: no match found
           23:  <p> Declare a method to calculate the sum of two numbers\</p> 
check:25'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:25'1      ?                                                              possible intended match
           24:  </div> 
check:25'0     ~~~~~~~~
           25:  </div> 
check:25'0     ~~~~~~~~
           26:  </div> 
check:25'0     ~~~~~~~~
           27:  </div> 
check:25'0     ~~~~~~~~
           28:  </div> 
check:25'0     ~~~~~~~~
            .
            .
            .
>>>>>>

--

********************
PASS: Clang Tools :: clang-doc/enum.cpp (9 of 11)
PASS: Clang Tools :: clang-doc/basic-project.test (10 of 11)
PASS: Clang Tools :: clang-doc/namespace.cpp (11 of 11)
********************
Failed Tests (1):
  Clang Tools :: clang-doc/comments-in-macros.cpp

You can see <p> Declare a method to calculate the sum of two numbers\</p> apper a \ in EOL.

If I change comment to

截屏2025-04-19 19 53 25

The test will all passed:

截屏2025-04-19 19 53 43

So should we change match content or \ in comment?

PS. I update code and you can see error log. I am waiting your reply to finish the final step:

  1. If you think we should remove \ in comment, I will remove it (As I said before, many code highlight process this \ to be a part of comment). The test will passed and generated content is also well.
  2. If you think we should keep \ in comment, the work may be too large. We may need to modify many highlighter, even laxer or parser. I don't think this is a good idea. Is there any standard of comment we can refer to?

Copy link
Contributor

@ilovepi ilovepi May 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I believe I mentioned that the file name didn't match in an earlier comment.

This patch is still not passing on CI, so please make it pass. I don't quite remember the parsing rules, so you'll either need to confirm the behavior by checking the AST, or by looking at the preprocessed output. I'm fine w/ any of the above if the test passes. We can always fix clang-doc behavior later if we don't like something about it or address it in clang's parsing.

Copy link
Contributor Author

@ZhongUncle ZhongUncle May 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine w/ any of the above if the test passes.

OK, I update code and passed CI. Please check.

DECLARE_METHODS
};

Loading