[Driver] Disallow -autolink-force-load with -incremental #15813
Conversation
6af333f changed the implementation of -autolink-force-load to only generate one symbol, but the /placement/ of that one symbol depends on the order of input files, and -incremental supports adding a file without rebuilding all other files. We don't have any need for these two to play nice together right now, so just disallow it.
|
@swift-ci Please smoke test |
lib/Driver/Driver.cpp
Outdated
| forceLoadArg->getSpelling(), | ||
| incrementalArg->getSpelling()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I would suggest pulling this block (204-214) out into a separate (static) function. I know it doesn't do much, just diagnoses a problem. But as is, it lengthens an already-too-long-for-quick-comprehension function. Ideally, every such step in this validate function would be it's own subroutine, making it possible to read it as a series of steps (validateThis(); validateThat();) at a glance. IMO, such a transformation would be worthwhile in terms of effort consumed vs future benefit to new maintainers.
There was a problem hiding this comment.
I still don't see this. Why is it better to have five separate validations broken up into separate functions? They are not reusable components and are called in exactly one place.
|
Review ping! |
|
The requirements:
I guess we could make things more complicated and say "if the 'first' file for an incremental build invocation changes, rebuild both the old and the new one", but honestly I'm not too worried, because no one should really be using -autolink-force-load outside of the overlays, and the overlays are (currently) never built incrementally. Heck, we could drop it completely on non-Apple platforms and no one would notice. |
|
(By the way, David convinced me offline to split up |
No functionality change.
|
@swift-ci Please smoke test |
There was a problem hiding this comment.
I really like the factoring of validateArgs. Thanks! One small thought concerning the comment 'Perform miscellaneous early validation of arguments.': add a line something like: "Issue diagnostics for any problems found." But not strictly necessary if you don't feel it's so.
|
An afterthought: maybe comments on functions are better if they don't use the same words as the function name. For instance the validateArgs comment could be "Check arguments for consistency" as opposed to "Validate the arguments". |
#15647 changed the implementation of -autolink-force-load to only generate one symbol, but the placement of that one symbol depends on the order of input files, and -incremental supports adding a file without rebuilding all other files. We don't have any need for these two to play nice together right now, so just disallow it.