Open Alexolut opened 1 week ago
What does the review summary comment say? The format-review
feature should attach a full diff of desired changes in the review's summary comment. It would be collapsed under the text "Click here for the full clang-format patch". For example: https://cpp-linter.github.io/cpp-linter-action/images/format-review.png
I have to ask, what triggers are used by your CI workflow? I think I asked you this before but got no answer. For example the triggers are under the yaml field on
:
on:
pull_request:
branches: [main, master, develop]
paths: ['**.c', '**.cpp', '**.h', '**.hpp', '**.cxx', '**.hxx', '**.cc', '**.hh', '**CMakeLists.txt', 'meson.build', '**.cmake']
push:
branches: [main, master, develop]
paths: ['**.c', '**.cpp', '**.h', '**.hpp', '**.cxx', '**.hxx', '**.cc', '**.hh', '**CMakeLists.txt', 'meson.build', '**.cmake']
Also, which lines are changed in the following code?
void TestFunc(const std::string& data) try {
do_something();
do_something_more();
} catch (const std::exception& ex) {
log_error(ex);
}
In general I don't have problems with other code. That's why I think the triggers not relevant here.
Only the line with catch
is problematic from cpp-linter point of view. All lines of code that I mentioned are new. So the entire function is checked.
If you don't answer all my questions, then I'm unable to help/diagnose the problem. I'm guessing this has occurred on a private repo.
formatted locally with clang-format
Are you also using clang-format v18 locally?
what triggers are used by your CI workflow?
This is important because the review may not be updated based on what event triggers the workflow.
What does the review summary comment say?
I'm curious if the "full clang-format patch" includes the line about catch
.
The repo is private. Excerpt from related yaml file:
on:
workflow_call:
I formatted entire file locally with clang-format 18.1.7 (on CI I see 18.1.8 but I had faced similar problem before while CI had 18.1.7 as well) before push to PR.
When I changed code into this (added extra spaces inside of parentheses):
void TestFunc(const std::string& data) try {
do_something();
do_something_more( );
} catch (const std::exception& ex) {
log_error( ex );
}
I got a message with diff in PR review:
I.e. no problems on line with catch
are detected in CI side! That's why I think triggers and difference in minor versions of LLVM are not relevant in my case.
When I apply the diff (i.e. format file again locally) I will see:
catch
The only way I found to work around the problem (get no complains from linter side) - don't use function-try-block and use regular try-block like this:
void TestFunc(const std::string& data) {
try {
do_something();
do_something_more();
} catch (const std::exception& ex) {
log_error(ex);
}
}
on: workflow_call:
So it is a reusable workflow. What are the triggers for the actual workflow that invokes the reusable workflow? I ask because reusable workflows inherit the event type from the calling workflow.
I formatted entire file locally with clang-format 18.1.7 (on CI I see 18.1.8 but I had faced similar problem before while CI had 18.1.7 as well) before push to PR.
I was more concerned about a difference between major versions. I doubt this problem is occurring from a difference between patch versions
When I apply the diff (i.e. format file again locally) I will see:
- no diff on review page
- complaints about line with catch
It sounds like format-review
is working as expected, but you keep seeing a message from the file-annotations
feature. Since this isn't the first time you have had complaints about the file-annotations
feature (and you're more focused on diff suggestions), I would recommend you disable it:
file-annotations: false
[!note] We have no way of deleting an outdated file annotation because they are only relevant to the commit that created the annotation. This usually isn't a problem because refreshing the list of file changes in a PR will only show annotations created by the latest commit to the head branch.
It could be that the line number in the annotation is incorrect, but this is highly unlikely.
I disabled annotation as you suggested file-annotations: false
. Below the output from action run:
Performing checkup on filename.cpp
INFO:CPP Linter:Running "/usr/bin/clang-format-18 -style=file --output-replacements-xml --lines=96:103 filename.cpp"
INFO:CPP Linter:Getting fixes with "/usr/bin/clang-format-18 -style=file --lines=96:103 filename.cpp"
Posting comment(s)
INFO:CPP Linter:1 clang-format-checks-failed
INFO:CPP Linter:0 clang-tidy-checks-failed
INFO:CPP Linter:1 checks-failed
ERROR:CPP Linter:response returned 403 from PUT https://api.github.com/repos/*/reviews/*/dismissals with message: {"message":"Branch protections do not permit dismissing this review","documentation_url":"https://docs.github.com/rest/pulls/reviews#dismiss-a-review-for-a-pull-request","status":"403"}
ERROR:CPP Linter:response returned 403 from PUT https://api.github.com/repos/*/reviews/*/dismissals with message: {"message":"Branch protections do not permit dismissing this review","documentation_url":"https://docs.github.com/rest/pulls/reviews#dismiss-a-review-for-a-pull-request","status":"403"}
As far as I understand the line INFO:CPP Linter:1 clang-format-checks-failed
means that the problem persist. Do have an ability (public sandbox?) to run your cpp-linter with my code snippet?
Do have an ability (public sandbox?) to run your cpp-linter with my code snippet?
No. We would need more than just the snippet:
You can enable cpp-linter's debug output:
verbosity: debug # default is 'info'
If I recall correctly, this should show the output from
/usr/bin/clang-format-18 -style=file --lines=96:103 filename.cpp
Look like the output is empty line? But same output here was before changing verbosity to debug.
the .clang-format config file you are using
I hope that default based on Google-style will be enough to reproduce
I forgot, clang-format output is basically the modified source. With format-review
enabled, the modified source is captured into a buffer that is used to generate a diff (which corresponds to the code suggestions).
Could this just be a difference in LF vs CRLF? git
typically normalizes line endings according to .gitattributes and git config settings
Could this just be a difference in LF vs CRLF?
I don't think so. All lines have the same ending. But complaining only for the one with catch
.
I would like to repeat that the problem not raising with regular try/catch block, only with function-try-block. Looks like clang-format (or other wrapper) returns non-zero exit code to cpp-linter, but doesn't suggest any changes.
If clang-format was returning a non-zero exit code, then it would have been reported with debug output enabled.
logger.info('Running "%s"', " ".join(cmds))
results = subprocess.run(cmds, capture_output=True)
if results.returncode:
logger.debug(
"%s raised the following error(s):\n%s", cmds[0], results.stderr.decode()
)
(from src here)
What is the output when you run the following command locally?
clang-format --output-replacements-xml -style=file --lines=96:103 filename.cpp
(assuming clang-format-18
is synonymous with a clang-format
call)
I'm curious to see what the XML output is. It might be that v18 uses a format that is different from previous versions' XML output (we've seen that problem before).
I hope that default based on Google-style will be enough to reproduce
Its not. You clearly use 4-space indents, Google guidelines mandate 2-space indents. I wouldn't be surprised if you deviate from their guidelines further.
output-replacement-xml returns me the following:
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='2789' length='0'> </replacement>
</replacements>
Offset 2789 points right after the curly brace:
} catch (const std::exception& ex) {
^
I'm not sure what does it mean. How to read this xml-output.
You clearly use 4-space indents
I mean that for the sake of reproducibility it (I hope) should not depend on .clang-format style. Just try to format my code snippet with your settings and look into response from cpp-linter, if it's not so hard for you.
Using the xml feedback that you provided, I saved this locally to a file named "fmt.xml". Then I opened a python interpretter to parse the xml similarly to how cpp-linter does:
>>> from pathlib import Path
>>> f = Path("fmt.xml").read_text(encoding="utf-8")
>>> f
"<?xml version='1.0'?>\n<replacements xml:space='preserve' incomplete_format='false'>\n<replacement offset='2789' length='0'> </replacement>\n</replacements>\n"
>>> import xml.etree.ElementTree as ET
>>> tree = ET.fromstring(f)
>>> len(tree)
1
>>> tree[0]
<Element 'replacement' at 0x7f0ce8f45170>
>>> tree[0].text
'\r\n '
>>> int(tree[0].attrib["offset"]) # the position where replacement begins
2789
>>> int(tree[0].attrib["length"]) # the number of characters to remove
0
Given that you said offset 2789 corresponds to the end of the catch( ... )
line, the suggested fix is appending \r\n
(4 spaces after a CRLF) after the {
.
I'm not sure what is happening in cpp-linter. I have to investigate the code to make sure that this case is covered. It certainly should be.
I mean that for the sake of reproducibility it (I hope) should not depend on .clang-format style. Just try to format my code snippet with your settings and look into response from cpp-linter, if it's not so hard for you.
Well, it wasn't trivial (I'm currently working from Windows), but all I was able to get was the following XML
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
</replacements>
where filename.cpp is
void TestFunc(const std::string& data) try {
do_something();
do_something_more();
} catch (const std::exception& ex) {
log_error(ex);
}
and .clang-format is
BasedOnStyle: google
IndentWidth: 4
Does you project use mixed line endings? Or only LF? Or only CRLF?
According your explanation (if I understand it correctly) looks like clang-format expects the following:
After that linter suggests me to remove entire line 100!
Mind blowing.
I work on Windows as well. So \r\n
only.
It looks like you did understand my analysis. This feels like it might be a bug in clang-format v18. I'm going to research how pygit2 (python binding we use for libgit2) deals with creating a patch from 2 buffers. The resulting patch is what cpp-linter uses to create code suggestions.
Can you double check the offset is translated correctly into line & column? It is a 1-based count.
You can do this with a python prompt:
from pathlib import Path
offset = 2789
filename = "filename.cpp" # replace this with the actual filename
ba = Path(filename).read_bytes()[:offset]
lines = ba.count(b"\n") + 1 # lines are a 1-based count
cols = offset - ba.rfind(b"\n")
print("line:", lines, "col:", cols)
This is an expanded version of the code that cpp-linter uses
You can do this with a python prompt
line: 99 col: 37
Exactly the same position that I mentioned before
} catch (const std::exception& ex) {
^
It seems that we can post an issue (bug) on LLVM GH only based on the assumption that we must get "empty" output-replacements-xml, e.g.:
<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
</replacements>
if we did clang-format with fix before on the same .clang-format settings. Am I right?
I would like to repeat that the problem not raising with regular try/catch block, only with function-try-block
I would focus on this fact.
The function-try-block is basically syntactic sugar for encapsulating the entire function body in a try-catch block. However, it is ideally used for a class' constructor that derives from a base class. Other use cases (like what you're doing) are really just a clever workaround for projects with restrictions on line length.
Am I right?
That is what an empty XML should look like, yes. If you do raise this upstream, then please leave a reference to this thread.
windows-latest
runner? This would inform git to default to CRLF on checkout, unless a .gitattributes file is being used.git config core.autocrlf
# or
git config --global core.autocrlf
Hmm, I'm looking at the diff flags that cpp-linter can use when creating a patch that gets translated into code suggestions in a PR review. Most diff flags don't really apply to our use case, which creates a patch from 2 arbitrary buffers, not anything in the index or working tree. However, I did find a flag that might be applicable to our specific usage:
GIT_DIFF_INDENT_HEURISTIC
: Use a heuristic that takes indentation and whitespace into account which generally can produce better diffs when dealing with ambiguous diff hunks.Although, I'm not sure if this flag would help avoid this problem (as reported in OP). I doubt it would hurt anything if I added it to cpp-linter.
This is definitely a bug in clang-format.
In cpp-linter, we use the XML output to count the suggested format fixes and represent it to the user as the clang-format-checks-failed
output. To workaround this, cpp-linter would have to compare the formatted source with the un-formatted source for each source file. Such a workaround would prove very costly in terms of memory consumption/runtime on large files and/or large repositories, and that's not even considering the extra post-processing needed to satisfy our lines-changed-only
and files-changed-only
options.
To work around this problem, I would suggest you either
I have code like this, formatted locally with clang-format. I use function-try-block here:
cpp-linter has complaints on this code, pointing to the line with
catch
(99):At the same time I don't see any diff suggestions in PR despite I have
format-review: true
and if I intentionally add wrongly-formatted changes on other lines - I will see only diff for those lines (but line withcatch
still won't be in diff for some reason).Full list of options that I use:
Looks like everything is OK with formatting, but cpp-linter mark that code as not-formatted.