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

Fix 21530 - dtoh: Sanitize identifiers wrt. reserved C++ keywords #12122

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

MoonlightSentinel
Copy link
Contributor

Extends the check for C++ keywords to append an underscore instead of
raising an error whenever the actual name doesn't matter for binary
compatibility.

CC @kinke @ibuclaw

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21530 normal dtoh: Identifiers need to be sanitized wrt. reserved C++ keywords

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12122"

@@ -3562,7 +3562,7 @@ class ToCppBuffer final : public Visitor
void visit(AliasDeclaration* ad);
void visit(Nspace* ns);
void visit(CPPNamespaceDeclaration* ns);
void handleNspace(Dsymbol* namespace, Array<Dsymbol* >* members);
void handleNspace(Dsymbol* namespace_, Array<Dsymbol* >* members);
Copy link
Member

@ibuclaw ibuclaw Jan 10, 2021

Choose a reason for hiding this comment

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

Could we just not emit parameter names? They are just superfluous information at best.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sanitizing here as well makes sense IMO, especially as all comments are stripped, so documentation is scarce already.

Copy link
Contributor

@RazvanN7 RazvanN7 Jan 11, 2021

Choose a reason for hiding this comment

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

Hmmm, maybe it would be useful if we could generate the documentation for the C++ headers also? We could simply paste the initial comment

@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jan 12, 2021
@RazvanN7 RazvanN7 closed this Jan 13, 2021
@RazvanN7 RazvanN7 reopened this Jan 13, 2021
Extends the check for C++ keywords to append an underscore instead of
raising an error whenever the actual name doesn't matter for binary
compatibility.
@MoonlightSentinel
Copy link
Contributor Author

(Rebased to resolve the merge conflicts)

@kinke kinke added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Jan 14, 2021
@dlang-bot dlang-bot merged commit b40c9bc into dlang:master Jan 14, 2021
@MoonlightSentinel MoonlightSentinel deleted the dtoh-fixup-identifiers branch January 14, 2021 23:37
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.

5 participants