halcompile: warn about, and reject colliding, mangled HAL names - #4299
Open
tzuohann wants to merge 1 commit into
Open
halcompile: warn about, and reject colliding, mangled HAL names#4299tzuohann wants to merge 1 commit into
tzuohann wants to merge 1 commit into
Conversation
A name declared in a .comp file is a C identifier, but it is exported
under a mangled HAL identifier: underscores become dashes and a trailing
dash or period is removed (comp.adoc, HALNAME). Nothing said so at
compile time, so "pin in float my_input" silently became
component.N.my-input.
Worse, check_name_ok() compares only declared names. Two declarations
that mangle to the same HAL name -- x_y and x_y_, the two rows of the
HALNAME table that share a HAL identifier -- therefore compiled cleanly
and failed much later, at load time:
HAL: ERROR: duplicate variable 'collide.0.x-y'
collide: rtapi_app_main: Invalid argument (-22)
Add check_hal_name(), which rejects that collision at the offending
line, and a once-per-file warning listing the names whose HAL identifier
differs from the declaration. Both messages point at the HALNAME
documentation. The warning is suppressed by -N (--no-name-warnings),
which the in-tree component rules pass, since those names are deliberate.
All 119 in-tree .comp files preprocess with no new error, and silently
under -N. tests/halcompile/halname covers the warning, -N, and the
rejected collision.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A name declared in a
.compfile is a C identifier, but it is exported under a mangled HAL identifier: underscores become dashes, and a trailing dash or period is removed. This is documented —docs/src/hal/comp.adoc, HALNAME under Syntax — but nothing says so at the point of use:halcompilecompiles that without a word, andhalcompile(1)has no naming section, so the author's first sign of it is a HAL file that does not work.The same mangling has a sharper edge.
check_name_ok()(halcompile.g:212) compares only declared names, so two declarations that mangle to one HAL name pass silently.x_yandx_y_are exactly the two rows of the HALNAME table incomp.adocthat share a HAL identifier:The failure names neither the
.compfile, nor the line, nor the rule that caused it. Reproduced on 2.9.8 as shipped by Debian, and the code is unchanged at2.9HEAD (to_hal()athalcompile.g:262).Change
check_hal_name()— a HAL-namespace counterpart tocheck_name_ok(). Two declarations that export the same HAL name are now an error at the offending line, instead of aloadrtfailure later.A once-per-file warning listing declared names whose HAL identifier differs, e.g.
Both messages point at the documentation that describes the mangling.
-N,--no-name-warningssuppresses that warning.src/hal/components/Submakefilepasses it on the two--documentrules and theobjects/%.crule, since in-tree HAL names are deliberate, so the build stays as quiet as it is today.Docs: a
NAMESsection indocs/man/man1/halcompile.1; a note under the HALNAME table incomp.adoc.Component names are left alone —
loadrt my_compkeeps its underscores while its pins appear undermy-comp.N., which the newNAMESsection spells out but does not change.If
2.9should stay strictly bugfix-only, the collision check is the bugfix and the warning plus-Ncan be dropped from this PR on request; they are separable hunks.Compatibility
The error only fires on a component that cannot load today, so no currently working
.compbecomes unbuildable. All 119 in-tree.compfiles preprocess with no new error; 64 of them would print the informational warning, which is why the in-tree rules pass-Nand a full build gains no new output.Testing
tests/halcompile/halname/— two minimal components covering the warning,-Nsilencing it, and the rejected collision (error text, non-zero exit, no.cproduced).tests/halcompile/namesstill passes unchanged. The man page renders clean throughgroff -man.Note
Companion PR for
master: #4298. It is the same change adapted to master's layout (halcompile.1.adocinstead of troff, the--adocrule,type_/dir_parameter names) — the branches are maintained separately, so neither supersedes the other.🤖 Generated with Claude Code