-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Add polonius compare mode #51138
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add polonius compare mode #51138
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
1207f82
to
539a6f0
Compare
if !path.exists() && self.config.compare_mode.is_some() { | ||
// fallback! | ||
|
||
if !path.exists() { |
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.
Hmm, this is a bit more hard-coded than I expected, but I imagine it does the right thing.
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.
Maybe it's not worth making a super general mechanism here, given that we'll hopefully be removing polonius and nll "soon".
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.
What I expected was some kind of table where, for each compare-mode, we specify a "parent" mode (which may be None
for nll) — and we iterate through modes, walking to their parents (and ultimately to a None
mode) — looking for a path that exists.
src/bootstrap/test.rs
Outdated
if let Some(compare_mode) = builder.config.cmd.compare_mode() { | ||
cmd.arg("--compare-mode").arg(compare_mode); | ||
} | ||
|
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.
There appears to be some very similar logic a few lines below:
It seems like there is a compare_mode
that comes from the test directory configuration as well as one (now) that you have added to builder.config.cmd
. Perhaps we want to just change this line to something like this:
let compare_mode = builder.config.cmd.compare_mode().or(self.compare_mode);
(This is giving precedence to the --compare-mode
option from the command line, which seems appropriate.)
(Alternatively, we could error if compare-mode is given both in the command line and the test suite definition; I think that the latter (test suite) can only occur when running ./x.py test
, in which case the command line probably ought not to be in use.)
539a6f0
to
ecb7f52
Compare
Have just force pushed this again. Still needs #51133 merged. |
ecb7f52
to
c0f897d
Compare
c0f897d
to
74d48ed
Compare
@bors r+ |
📌 Commit 74d48ed has been approved by |
🔒 Merge conflict |
74d48ed
to
a73b4d7
Compare
@bors r=pnkfelix |
📌 Commit a73b4d7 has been approved by |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors r- |
(Travis failure) |
a73b4d7
to
214c25d
Compare
214c25d
to
b39a1d6
Compare
@bors r=pnkfelix |
📌 Commit b39a1d6 has been approved by |
Add polonius compare mode **This is now ready to review/merge**
☀️ Test successful - status-appveyor, status-travis |
This is now ready to review/merge