Skip to content
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

[clang-tidy][NFC][doc] Improve documentation for modernize-use-equals… #65231

Merged
merged 7 commits into from
Sep 4, 2023

Conversation

carlosgalvezp
Copy link
Contributor

@carlosgalvezp carlosgalvezp commented Sep 3, 2023

…-delete

So the purpose of the check is more clear. Update examples code to show compliant code.

Fixes #65221

…-delete

So the purpose of the check is more clear. Update examples code to
show compliant code.

Fixes llvm#65221
Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

Maybe:

Identifies unimplemented private special member functions, recommends using '= delete' for them, and suggests relocating such deleted functions from the private to the public section.

Before the introduction of C++11, the primary method to effectively "erase" a particular function involved declaring it as private without providing a definition. This approach would result in either a compiler error (when attempting to call a private function) or a linker error (due to an undefined reference).

However, subsequent to the advent of C++11, a more conventional approach emerged for achieving this purpose. It involves flagging functions as "= delete" and retaining them within the public section of the code.

To prevent false positives, this check is only applicable within a translation unit where all other member functions have been implemented. The check will generate partial fixes by introducing "= delete," but the user is responsible for manually relocating it to the public section.

@PiotrZSL
Copy link
Member

PiotrZSL commented Sep 3, 2023

Other interesting thing is that currently this check is limited just to LangOpts.CPlusPlus, when it should be to LangOpts.CPlusPlus11. And in such case documentation could mention that requires C++11 or later.

@carlosgalvezp
Copy link
Contributor Author

Yep, we could certainly change it to C++11 or later, but I think it's out of the scope for this NFC patch. Regarding documentation, I haven't seen that we document this detail in other checks, it seems like an implementation detail that could cause maintenance effort if documented.

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

LGTM

@PiotrZSL
Copy link
Member

PiotrZSL commented Sep 3, 2023

Do we want to sync comment above check class in code with first sentence in documentation ?

@carlosgalvezp
Copy link
Contributor Author

Yes good point, will fix!

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

No more comments from me.

@carlosgalvezp carlosgalvezp merged commit fdb6e8b into llvm:main Sep 4, 2023
1 check passed
@carlosgalvezp carlosgalvezp deleted the modernize_use_equals_delete branch September 4, 2023 09:45
avillega pushed a commit to avillega/llvm-project that referenced this pull request Sep 11, 2023
llvm#65231)

…-delete

So the purpose of the check is more clear. Update examples code to show
compliant code.

Fixes llvm#65221

---------

Co-authored-by: Carlos Gálvez <carlos.galvez@zenseact.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclear documentation of modernize-use-equals-delete
3 participants