Skip to content

Conversation

jurahul
Copy link
Contributor

@jurahul jurahul commented Oct 15, 2025

Extend CS rule to use namespace qualifiers to define previously declared functions to variables and classes as well.

Extend CS rule to use namespace qialifiers to define previously
declared functions to variables as well.
@jurahul jurahul marked this pull request as ready for review October 15, 2025 18:16
Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

I think we might as well generalize this over all declarations, rather than specifying only functions and variables (we shouldn't have many variables declared in headers anyway, but similarly, might have a small number of opaque classes declared in headers too)

@jurahul
Copy link
Contributor Author

jurahul commented Oct 15, 2025

Note that this specific section is not about requiring declarations in headers but about using namespace qualifiers in definitions. Are you suggesting that it can be used/should be used for opaque classes like RecordKeeperImpl in TableGen? I just tested it out and it works, so yeah, I can extend the example to cover opaque classes that are declared in LLVM namespace in a header but defined in .cpp files.

@jurahul
Copy link
Contributor Author

jurahul commented Oct 15, 2025

Done, added example of an opaque class and changed the language to be more general.

@jurahul jurahul requested a review from dwblaikie October 15, 2025 22:31
@jurahul
Copy link
Contributor Author

jurahul commented Oct 16, 2025

Thanks, I'll wait for other folks to chime in as well before merging.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

@jurahul
Copy link
Contributor Author

jurahul commented Oct 17, 2025

@nikic will wait for you approval as well before merging.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@jurahul jurahul changed the title [LLVM][CodingStandard] Extend namespace qualifier rule to variables [LLVM][CodingStandard] Extend namespace qualifier rule to variables/classes Oct 20, 2025
@jurahul jurahul merged commit 58dd7a6 into llvm:main Oct 20, 2025
8 of 10 checks passed
@jurahul jurahul deleted the cs_variable_defs branch October 20, 2025 16:34
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.

4 participants