Page MenuHomePhabricator

[clang] Allow lexer to handle string_view literals
ClosedPublic

Authored by AntonBikineev on Nov 17 2016, 7:01 PM.

Details

Summary

This is to address p0403r0 proposal.

Diff Detail

Event Timeline

AntonBikineev retitled this revision from to [clang] Allow lexer to handle string_view literals.
AntonBikineev updated this object.
EricWF added a subscriber: EricWF.Nov 17 2016, 10:57 PM

Please add tests to cxx1y-user-defined-literals.cpp (and probably rename it).

Other than that this LGTM, but I don't feel comfortable approving clang patches.

@mclow.lists is working on this in D26667.
The review comments from that apply here too.

Should this review be closed then?

AntonBikineev updated this revision to Diff 78510.
AntonBikineev updated this object.

Just added a small test case

Does Sema::CheckLiteralOperatorDeclaration need to check StringLiteralParser::isValidUDSuffix?

Does Sema::CheckLiteralOperatorDeclaration need to check StringLiteralParser::isValidUDSuffix?

Thanks, nice point! Just addressed it.

rsmith added inline comments.Nov 18 2016, 5:32 PM
include/clang/Basic/DiagnosticLexKinds.td
189–190 ↗(On Diff #78602)

I don't see a need for a custom diagnostic for this. Using our normal "reserved ud-suffix" diagnostic seems fine, and it doesn't make sense to treat this suffix as a hard error but accept all the rest with a warning.

lib/Lex/LiteralSupport.cpp
1716–1717

Just make this call NumericLiteralParser::isValidUDSuffix and then check for the sv case. All the numeric suffixes are also valid string literal suffixes for the form operator""suffix.

AntonBikineev added inline comments.Nov 19 2016, 2:36 AM
lib/Lex/LiteralSupport.cpp
1716–1717

This makes sense for the call sites we currently have.

All the numeric suffixes are also valid string literal suffixes for the form operator""suffix.

Don't really understand this part. It seems inconsistent if one calls, say,

StringLiteralParser::isValidUDSuffix(LangOpts(), "if")

and gets

true

Richard, thanks, I addressed your comments.

rsmith accepted this revision.Nov 21 2016, 9:33 AM
rsmith edited edge metadata.

LGTM, thanks!

lib/Lex/Lexer.cpp
1717

I don't think the LangOpts variable is worthwhile; remove and replace the use of it below with getLangOpts().

This revision is now accepted and ready to land.Nov 21 2016, 9:33 AM
AntonBikineev edited edge metadata.
AntonBikineev marked an inline comment as done.Nov 23 2016, 1:15 AM

@rsmith Richard, any plans to merge this or is there anything left?

EricWF closed this revision.Dec 29 2016, 9:02 PM

Committed on behalf of @AntonBikineev in r290744.