Skip to content

[NFC][TableGen] DirectiveEmitter code cleanup #107775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

jurahul
Copy link
Contributor

@jurahul jurahul commented Sep 8, 2024

Eliminate unnecessary llvm:: prefix as this code is in llvm namespace.
Use ArrayRef<> instead of std::vector references when appropriate.
Use .empty() instead of .size() == 0.

@jurahul jurahul force-pushed the directive_emitter_cleanup branch from 77a95d1 to c992e2b Compare September 8, 2024 20:48
@jurahul jurahul marked this pull request as ready for review September 9, 2024 00:52
@kazutakahirata
Copy link
Contributor

Thank you for the cleanup. Now, is there any way you could separate the {}->() conversions with the rest? A {}->() conversion can have a surprising effect like std::vector<int> Vec(3,5) vs std::vector Vec{3, 5}, so I would have to be more careful and understand each type's constructor than, say, when reviewing comments or removal of llvm::.

@jurahul
Copy link
Contributor Author

jurahul commented Sep 9, 2024

Thank you for the cleanup. Now, is there any way you could separate the {}->() conversions with the rest? A {}->() conversion can have a surprising effect like std::vector<int> Vec(3,5) vs std::vector Vec{3, 5}, so I would have to be more careful and understand each type's constructor than, say, when reviewing comments or removal of llvm::.

I may be, though all the {} -> () changes are for constructors with single arguments here, and for classes defined in the DirectiveEmitter.h, so the risk of the issues you mentioned above is less. But I can if you insist.

Eliminate unnecessary llvm:: prefix as this code is in llvm namespace.
Use () for constructors instead of {} (per LLVM coding standards).
Use ArratRef<> instead of std::vector references when appropriate.
Use .empty() instead of .size() == 0.
@jurahul jurahul force-pushed the directive_emitter_cleanup branch from c992e2b to 889e0dd Compare September 9, 2024 17:01
@jurahul
Copy link
Contributor Author

jurahul commented Sep 9, 2024

Ok, I removed the {} -> () change. @kazutakahirata PTAL.

Copy link
Contributor

@kazutakahirata kazutakahirata left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jurahul jurahul merged commit 78c1009 into llvm:main Sep 9, 2024
8 checks passed
@jurahul jurahul deleted the directive_emitter_cleanup branch September 9, 2024 18:35
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.

2 participants