compiletest: Migrate TestProps directive handling to a system of named handlers
#147955
Conversation
|
Some changes occurred in src/tools/compiletest cc @jieyouxu
|
rustbot
added
A-compiletest
rustbot
added
S-waiting-on-review
|
Running a bunch of try jobs to check for unforeseen problems: @bors try jobs=x86_64-msvc-1,i686-msvc-1,x86_64-mingw-1,test-various,armhf-gnu,aarch64-apple,dist-i586-gnu-i586-i686-musl |
compiletest: Migrate `TestProps` directive handling to a system of named handlers try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1 try-job: test-various try-job: armhf-gnu try-job: aarch64-apple try-job: dist-i586-gnu-i586-i686-musl
This comment has been minimized.
This comment has been minimized.
|
I'll review this |
|
☔ The latest upstream changes (presumably #148305) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
| props.compile_flags.extend(flags); | ||
| } | ||
| }), | ||
| handler("edition", |config, ln, props| { |
There was a problem hiding this comment.
Discussion: for the fixed directive names, i.e. the edition, revisions, pp-exact and normalize-*, should we also introduce a named constant for them?
I was mostly thinking if we might be able to document them alongside the revision impl via rustdoc, but that won't work necessarily for 'dynamic' directive names.
There was a problem hiding this comment.
I was thinking of eventually moving away from the named constants, at least for the vast majority of handlers where once the directive name has been matched, the handler body no longer cares what the actual name was.
Currently most of the handlers mention the directive name twice, once for matching and once again when calling the appropriate parse method. But I see that as a temporary arrangement, because overhauling the individual parse methods was out of scope for this PR.
There was a problem hiding this comment.
Hm yeah. That does make a lot of sense. This is perfectly fine for this PR 👍
|
You can r=me once addressing the nits. |
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
This step consists of two changes: - Renaming `self` to `props` - Inserting temporary comments to preserve line breaks This will make it easier to verify that the main migration commit preserves all of the lines being migrated.
Use `git diff --color-moved --color-moved-ws=ignore-all-space` (or similar) to verify that the directive-processing lines have been moved without changes.
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Resolved conflict with #148510, which also makes the test a bit nicer since we can just use |
|
@bors r=jieyouxu |
bors
added
S-waiting-on-bors
compiletest: Migrate `TestProps` directive handling to a system of named handlers One of the very silly things about directive processing in compiletest is that for each directive in the test file, we proceed to check it against dozens of different directive names in linear sequence, without any kind of indexed lookup, and without any early-exit after a known directive name is found (unless a panic occurs). This PR is a big step away from that, by taking the `iter_directives` loop in `TestProps::load_from` and making all of its directive processing dispatch to a hashtable of individual name-specific handlers instead. --- The handler system is set up in a way that should allow us to add capabilities or change the implementation as needed, without having to mass-modify the existing handlers (e.g. this is why the `handler` and `multi_handler` functions are used). --- This PR is focused on mass-migrating all of the `TestProps` directive processing into handlers. Most of the resulting handlers could obviously be simplified further (e.g. by avoiding the redundant name checks that were needed in the pre-migration code), but I've avoided doing any such simplifications in this PR to keep its scope limited and make reviewing easier. The patches in this PR have been arranged so that the main migration can be inspected with `git diff --color-moved --color-moved-ws=ignore-all-space` to verify that it moves all of the relevant lines intact, without modifying or discarding any of them. r? jieyouxu
compiletest: Migrate `TestProps` directive handling to a system of named handlers One of the very silly things about directive processing in compiletest is that for each directive in the test file, we proceed to check it against dozens of different directive names in linear sequence, without any kind of indexed lookup, and without any early-exit after a known directive name is found (unless a panic occurs). This PR is a big step away from that, by taking the `iter_directives` loop in `TestProps::load_from` and making all of its directive processing dispatch to a hashtable of individual name-specific handlers instead. --- The handler system is set up in a way that should allow us to add capabilities or change the implementation as needed, without having to mass-modify the existing handlers (e.g. this is why the `handler` and `multi_handler` functions are used). --- This PR is focused on mass-migrating all of the `TestProps` directive processing into handlers. Most of the resulting handlers could obviously be simplified further (e.g. by avoiding the redundant name checks that were needed in the pre-migration code), but I've avoided doing any such simplifications in this PR to keep its scope limited and make reviewing easier. The patches in this PR have been arranged so that the main migration can be inspected with `git diff --color-moved --color-moved-ws=ignore-all-space` to verify that it moves all of the relevant lines intact, without modifying or discarding any of them. r? jieyouxu
4 participants
One of the very silly things about directive processing in compiletest is that for each directive in the test file, we proceed to check it against dozens of different directive names in linear sequence, without any kind of indexed lookup, and without any early-exit after a known directive name is found (unless a panic occurs).
This PR is a big step away from that, by taking the
iter_directivesloop inTestProps::load_fromand making all of its directive processing dispatch to a hashtable of individual name-specific handlers instead.The handler system is set up in a way that should allow us to add capabilities or change the implementation as needed, without having to mass-modify the existing handlers (e.g. this is why the
handlerandmulti_handlerfunctions are used).This PR is focused on mass-migrating all of the
TestPropsdirective processing into handlers. Most of the resulting handlers could obviously be simplified further (e.g. by avoiding the redundant name checks that were needed in the pre-migration code), but I've avoided doing any such simplifications in this PR to keep its scope limited and make reviewing easier.The patches in this PR have been arranged so that the main migration can be inspected with
git diff --color-moved --color-moved-ws=ignore-all-spaceto verify that it moves all of the relevant lines intact, without modifying or discarding any of them.r? jieyouxu