Implicit deps resolution: add #embed and fix #include followed by a comment - #4889
Implicit deps resolution: add #embed and fix #include followed by a comment#4889vassilit wants to merge 2 commits into
Conversation
The C scanners recognised only #include and C++ #import, so a change to a resource named by #embed did not mark the embedding source out of date. Both scanners now track the resource. A quoted name is looked up relative to the including file, a bracketed name in $CPPPATH: - CScanner's regex captures the directive keyword, through a new private _ClassicCPPEmbed subclass which drops the keyword again when computing the sort key, so result ordering is unchanged. ClassicCPP itself keeps its documented three-group contract, which IDL, SWIG and RC rely on. - SCons/cpp.py, which backs the opt-in CConditionalScanner, gains the keyword in its directive table plus a do_embed handler, and honours #embed in the conditional-branch wiring of start_handling_includes and stop_handling_includes. The resource itself is deliberately not scanned: its contents are embedded as data (possibly binary) rather than preprocessed. Resolved resources are tagged on the node and filtered out by recurse_nodes(). Also fixes resolve_include() assuming the closing delimiter is the last character of the line, which turned `#embed "d.bin" limit(4)` into the filename `d.bin" limit(4`, and likewise mangled an #include carrying a trailing comment. Signed-off-by: Vassili Tchersky <vt+git@vbcy.org> Assisted-by: Claude Opus 5
|
Thanks for your attention. There are some issues already on missing support for features introduced in newer C++ standards (and C standards too, for that matter)... see for example #4517 and #4518. Aren't there feature test macros for |
Are you referring to __has_embed ? I use it as a feature test as well, like here. |
Yes. What happens if you use |
It fails, with |
| not be scanned for dependencies of its own. :func:`_scannable` drops | ||
| tagged nodes when a scanner recurses into what it found. | ||
| """ | ||
| node.attributes.embedded_resource = True |
There was a problem hiding this comment.
Tagging the resource "globally" like this seems to preclude the file in question ever being used another way in the project (say, via #include). Maybe that's fine, and nobody would ever do that, but maybe we should leave a note about that somewhere if that's an intentional limitation.
There was a problem hiding this comment.
If this nit matters, I think just dropping the embedded_resource attribute once it's seen ("consumed") in _scannable ought to do the trick. I'll let wiser folk decide if I'm just being silly.
There was a problem hiding this comment.
@vassilit -I think this is unnecessary?
If the file is xyz.bmp, there's no scanner for .bmp so it won't be scanned.
If the file is xyz.h, there's a scanner for that, and it will be scanned. (although as far as I understand the use model for #embed it wouldn't make sense to us that for a source file?
There was a problem hiding this comment.
Tagging the resource "globally" like this seems to preclude the file in question ever being used another way in the project (say, via
#include). Maybe that's fine, and nobody would ever do that, but maybe we should leave a note about that somewhere if that's an intentional limitation.
I'm not that sure that nobody would ever do that, but those who do may use explicit dependencies instead.
|
Is there a downside to having this when #embed isn't handled by the compiler? |
It would, but for some versions of the standard, this seems acceptable, e.g. C17/C18 section 6.10 paragraph 9 says: The execution of a non-directive preprocessing directive results in undefined behavior. |
That's the acceptable compiler behavior. Seems like if we knew we were dealing with a c23 or c++26 then we could enable this, otherwise disable it would provide a more accurate picture of the actual dependencies. What file suffixes are likely to be pulled in via embed? |
What I meant is if #embed is found unguarded in a source file is likely destined to be compiled with a compiler that supports it. If guarded by CPP, then yes, there is a risk of dependancies wanted only when compiled with a specific compiler and not wanted with an older one. But usually, the data is needed by the program, #embeded directly or not.
In our SCons-built project, we embed source code (.sh, .py, .go). But every file suffix is likely to be embeded, it's just data. There is another case, #embed with limit() can depend on only part of the file and we have no simple way of knowing which offsets of the file changed, so that can lead to uneeded rebuilds. |
|
One issue with your current solution, if the file is also a source, a "regular" implicit dependency, is it could be scanned recursively before it's Given the types of files you've stated could be embedded, does it really make sense to exclude the recursive dependency? Did you run into such causing an issue with your build, or were you being preemptively cautious? |
|
Just to explain the level of caution I'm taking with this PR.
|
3 participants
#embedC23 (and c++26) preprocessor feature. (see: here for an explanation of what this is#includeBoth a small bug-fix and a new feature.
Tested manually on rmlint:
Before this PR:
#include "header.h"in code.c, modify header.h, code.c get rebuilt.#include "header.h" /* useful header */in code.c, modify header.h, code.c do not get rebuilt.#embed "py.py"in code.c, modify py.py, code.c do not get rebuilt.After:
#include "header.h"in code.c, modify header.h, code.c get rebuilt.#include "header.h" /* useful header */in code.c, modify header.h, code.c do get rebuilt.#embed "py.py"in code.c, modify py.py, code.c do get rebuilt.Contributor Checklist:
CHANGES.txtandRELEASE.txt(and read theREADME.rst).