Skip to content

Conversation

@willieyz
Copy link
Contributor

@willieyz willieyz force-pushed the extend-macro-typo-MLK_XXX branch from d3bd7f6 to 55d5412 Compare December 26, 2025 11:00
Copy link
Contributor

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @willieyz. As I understand, with the change we would still allow MLK_FOO if there is #define MLK_FOO somewhere. That's not intended. We should not have any MLK_XXX or MLKEM_XXX macros in the mldsa-native code base.

@hanno-becker hanno-becker self-assigned this Dec 28, 2025
@willieyz willieyz force-pushed the extend-macro-typo-MLK_XXX branch from 55d5412 to a8f9ccc Compare December 28, 2025 15:01
@willieyz willieyz marked this pull request as ready for review December 29, 2025 04:21
@willieyz willieyz requested a review from a team as a code owner December 29, 2025 04:21
scripts/autogen Outdated
_RE_MARKDOWN_CITE = re.compile(r"\[\^(?P<id>\w+)\]")
_RE_C_CITE = re.compile(r"@\[(?P<id>\w+)")
_FORBIDDEN_PREFIXES = "MLK_|MLKEM_"
_RE_MACRO_CHECK = re.compile(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_RE_MACRO_CHECK is overwritten again on line 37.

I suggest you keep it simple and just add a _RE_MLKEM_MACRO_CHECK which like, _RE_MACRO_CHECK, just with MLK/MLKEM, and then you add a separate check for their presence rather than extending the existing one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, @hanno-becker , thank you for your review,
I had change the macro checking method according to your suggestion,
Now all checking logic related with MLK/MLKEM will be processing in separate part, and mange the macro in additional _RE_MLKEM_MACRO_CHECK, thank you for your help!

Copy link
Contributor

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

@willieyz willieyz force-pushed the extend-macro-typo-MLK_XXX branch 3 times, most recently from d6eb9f3 to 946bee7 Compare December 30, 2025 09:41
f"Likely typo {txt} in {filename}:{line_no}? Not a defined macro."
)

# Separate check for wrongly ported MLK/MLKEM macros
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you run this check first? Otherwise, if you wrongly port #define MLK_FOO ... but use MLD_FOO in the code, you'll get the error "Undefined macro MLD_FOO" from the first check, but it would be clearer to get the second error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @hanno-becker, thank you for your help.
You are correct. Thank you for pointing this issue out. I have adjusted the order based on your suggestion and verified it with selftests.

Copy link
Contributor

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

- This commit extend the macro tyop checker for catch wrongly port for
  MLK_XXX / MLKEM_XXX, by adding similar regex like _RE_MACRO_CHECK but
  with "MLK_|MLKEM_" and use it with a seperate check in
  check_macro_typos_in_file()

- Also, for handle the cases in defines, like:
  #define MLK_FOO,
  this commit add a additional checking in check_macro_typos_in_file(),
  with above checking method.

Signed-off-by: willieyz <[email protected]>
- This commit fixes a error cause by macro typo in .clang-format where MLK_INTERNAL_API
should be MLD_INTERNAL_API. This typo was detected after extending the
macro checker to catch wrongly ported MLK_XXX/MLKEM_XXX macros.

- The typo fix causes clang-format to reformat some files (packing.h and
  polyvec.h) by changing macro order from:
    MLD_MUST_CHECK_RETURN_VALUE
    MLD_INTERNAL_API
  to:
    MLD_MUST_CHECK_RETURN_VALUE MLD_INTERNAL_API

- This reordering is the wrong, and it is because the order of these two macor,
  after referece from mlkem-native, we switch the macro order from:
    MLD_MUST_CHECK_RETURN_VALUE
    MLD_INTERNAL_API
  to:
    MLD_INTERNAL_API
    MLD_MUST_CHECK_RETURN_VALUE
  This is correct formatting behavior and matches the mlkem-native convention.

Signed-off-by: willieyz <[email protected]>
- This commit fixes a error cause by macro typo in .clang-format
    where MLK_UNION_OR_STRUCT should be
    MLD_UNION_OR_STRUCT. This typo was detected after
    extending the macro checker to catch wrongly ported
    MLK_XXX/MLKEM_XXX macros.

Signed-off-by: willieyz <[email protected]>
@willieyz willieyz force-pushed the extend-macro-typo-MLK_XXX branch from 946bee7 to 94bacce Compare December 31, 2025 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants