This is an archive of the discontinued LLVM Phabricator instance.

[HLSL] Adjust access specifier behavior
ClosedPublic

Authored by beanz on Apr 26 2022, 4:51 PM.

Details

Summary

HLSL doesn't support access specifiers. This change has two components:

  1. Make default access for classes public
  2. Diagnose the use of access specifiers as a clang HLSL extension

As long as the default behavior for access specifiers matches HLSL,
allowing them to be used doesn't cause sourece incompatability with
valid code. As such enabling them as a clang extension seems like a
reasonable approach.

Fixes #55124

Diff Detail

Event Timeline

beanz created this revision.Apr 26 2022, 4:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 4:51 PM
beanz requested review of this revision.Apr 26 2022, 4:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 4:51 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay added inline comments.Apr 26 2022, 10:32 PM
clang/test/ParserHLSL/access_specifiers.hlsl
9

May need tests for protected: and public:, and possibly for struct.

aaron.ballman added inline comments.Apr 27 2022, 11:16 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1603

This also needs to go into a warning group.

clang/lib/Parse/ParseDeclCXX.cpp
3517–3519

Gross. 🤮

clang/lib/Sema/SemaDeclCXX.cpp
2504–2507

🤮

clang/test/ParserHLSL/access_specifiers.hlsl
5–6

(It's generally easier to maintain tests when the expected check lines are directly on the impacted line rather than using offsets to it. Same applies through the rest of the test. NB: we don't care about 80 col limits in tests.)

beanz updated this revision to Diff 425678.Apr 27 2022, 6:58 PM

Updates based on PR feedback.

Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2022, 6:58 PM
aaron.ballman accepted this revision.Apr 28 2022, 5:49 AM

LGTM aside from whitespace changes.

clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
1 ↗(On Diff #425678)

It looks like these are unintentional whitespace changes? (Same below)

This revision is now accepted and ready to land.Apr 28 2022, 5:49 AM
beanz added inline comments.Apr 28 2022, 8:09 AM
clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
1 ↗(On Diff #425678)

Ha! I didn't notice this came along for the ride. This is from me trying to get the git file attributes setup correctly. This file should be crlf in the index (hence the title and comment) but is lf.

This revision was automatically updated to reflect the committed changes.