-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[clang-doc] fix flaky test in clang-doc #101387
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.
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
[clang-doc] fix flaky test in clang-doc #101387
Conversation
This isn't fixing anything. Its re-enabling it. I'd also suggest adding people from that bug as reviewers to help you get this re-enabled. |
This was just meant as an experiment to check what was failing in the CI pipeline |
4543ee0
to
114a572
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
399c51e
to
b88d3ea
Compare
1630f5a
to
2539d74
Compare
@@ -104,6 +104,9 @@ struct Reference { | |||
|
|||
bool mergeable(const Reference &Other); | |||
void merge(Reference &&I); | |||
bool operator<(const Reference &Other) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How stable are usrs? Perhaps we should be sorting on another property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern here is that we probably want clang-doc to output to be ordered consistently. While sorting by USRs may make it deterministic, it’s also likely that they won’t be ordered similarly run to run. For instance, I’d expect public class fields to be sorted by name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main reason I used USR was because it was unique to every declaration std sort wouldn't deal with equal elements, but it seems from the doc llvm:;sort already deterministically sort equal elements So I changed it to sort by name
3b0c23b
to
3de5ffe
Compare
d4e62cb
to
fc2a139
Compare
In the description, its probably better to use the |
1b3ec71
to
49b39e6
Compare
bool operator<(const SymbolInfo &Other) const { | ||
if (DefLoc && Other.DefLoc) { | ||
return *DefLoc < *Other.DefLoc; | ||
} | ||
if (Loc.size() > 0 && Other.Loc.size() > 0) { | ||
return Loc[0] < Other.Loc[0]; | ||
} | ||
return extractName() < Other.extractName(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some comments about this comparison. It seems you're ordering by DefLoc, then Loc, and lastly by Name, but why that's a good/reasonable ordering isn't obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the logic up a bit, now it works by sorting by declaration location, location then name, I also added some comments explaining my logic
/// Make the output of clang-doc deterministic by sorting the children of | ||
/// namespaces and records. | ||
void sortUsrToInfo(llvm::StringMap<std::unique_ptr<doc::Info>> &USRToInfo) { | ||
for (auto &I : USRToInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does USRToInfo
itself need to be in a sorted/stable order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No since USRToInfo is generated on a per file basis so there's no need to sort it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't necessarily rule out its own iteration order affecting the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How so? StringMap iteration order is not guaranteed to be deterministic and I don't think it was affecting the tests before
if (Loc.size() > 0 && Other.Loc.size() > 0) { | ||
return Loc[0] < Other.Loc[0]; | ||
} | ||
return extractName() < Other.extractName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the overall comparison, you could probably use std::tuple
to make the logic simpler. The optional value may make that harder though, as I don't recall offhand how that's normally handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too familiar with what you are talking about can you give an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
61bf9a8
to
3768c86
Compare
3768c86
to
7255f85
Compare
// Sort by declaration location since we want the doc to be | ||
// generated in the order of the source code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation often sorts methods by name, so its easy to find things. As an example, here's Rust's Vec
documentation https://doc.rust-lang.org/std/vec/struct.Vec.html. Methods are sorted in the navigation window and within the body, despite not being that way in source.
Doxygen maintains source order, so its fine to do it that way for now, but we may want to consider making that a configurable option, since I think its more user friendly to do it that way (and arguably more modern).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but test locally w/ LLVM_ENABLE_EXPENSIVE_CHECKS=On https://llvm.org/docs/CMake.html. If that passes, then go ahead and land this, since I think the nondeterminism in output order will be accounted for.
Fixes #97507
#96809 introduce non-determinstic behaviour in clang-doc which caused the output to be randomly ordered which lead to randomly failing test. This patch modify clang-doc to behave deterministically again by sorting the output by location or USR.