[PATCH] Allow string literals as module identifiers

[Daniel Jasper]

================
Comment at: docs/Modules.rst:243
@@ -242,3 +242,3 @@
 -----------------
-Module map files use a simplified form of the C99 lexer, with the same rules for identifiers, tokens, string literals, ``/* */`` and ``//`` comments. The module map language has the following reserved words; all other C identifiers are valid identifiers.
+Module map files use a simplified form of the C99 lexer, with the same rules for identifiers, tokens, string literals, ``/* */`` and ``//`` comments. The module map language has the following reserved words; all other C identifiers are valid identifiers. Additionally, string literals can also be used to specify module names.
 
----------------
Richard Smith wrote:
> This seems out of place here.
Yeah.. I want to get feedback on the general design before spending much more time on the documentation (should have written that in the description). Thanks for the very detailed feedback here!!!

================
Comment at: docs/Modules.rst:267
@@ -266,3 +266,3 @@
   *module-id*:
     *identifier* ('.' *identifier*)*
 
----------------
Richard Smith wrote:
> Please update the grammar for this feature: change `identifier` to `module-name` here and add a grammar rule:
> 
>   *module-name*:
>     *identifier*
>     *string-literal*
> 
> ... and some words specifying that `identifier` is equivalent to `"identifier"`.
Done.

================
Comment at: docs/Modules.rst:275-279
@@ -274,7 +274,7 @@
 
   *module-declaration*:
     ``explicit``:sub:`opt` ``framework``:sub:`opt` ``module`` *module-id* *attributes*:sub:`opt` '{' *module-member** '}'
     ``extern`` ``module`` *module-id* *string-literal*
 
 The *module-id* should consist of only a single *identifier*, which provides the name of the module being defined. Each module shall have a single definition. 
 
----------------
Richard Smith wrote:
> Change `*module-id*` to `*module-name*` here and remove the redundant "only a single *identifier*" restriction.
Done.

================
Comment at: docs/Modules.rst:499-501
@@ -498,5 +498,5 @@
   *wildcard-module-id*:
     *identifier*
     '*'
     *identifier* '.' *wildcard-module-id*
 
----------------
Richard Smith wrote:
> Change `*identifier*` to `*module-name*` here.
Done.

================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:617
@@ -616,3 +616,3 @@
 def error_undeclared_use_of_module : Error<
-  "use of a module not declared used: '%0'">;
+  "module %0 does not depend on a module exporting: '%1'">;
 
----------------
Richard Smith wrote:
> Remove colon here, please.
Done.

================
Comment at: lib/Lex/ModuleMap.cpp:1065
@@ -1065,2 +1064,3 @@
+    if (Tok.is(MMToken::Identifier) || Tok.is(MMToken::StringLiteral)) {
       Id.push_back(std::make_pair(Tok.getString(), Tok.getLocation()));
       consumeToken();
----------------
Richard Smith wrote:
> Maybe remove quotes here?
Tok.getString() already returns the string without quotes.

================
Comment at: lib/Lex/ModuleMap.cpp:978-981
@@ -977,6 +977,6 @@
     // Parse the string literal.
     LangOptions LangOpts;
     StringLiteralParser StringLiteral(&LToken, 1, SourceMgr, LangOpts, *Target);
     if (StringLiteral.hadError)
       goto retry;
     
----------------
Richard Smith wrote:
> Should we perform string literal concatenation here?
I don't know, but I'd hope that we don't need to. A single module name needing to be split over multiple lines seems .. wrong.

================
Comment at: lib/Lex/Preprocessor.cpp:750-757
@@ -751,1 +749,10 @@
+      (Result.is(tok::identifier) || Result.is(tok::string_literal))) {
+    if (Result.is(tok::string_literal)) {
+      // String literals are valid module identifiers. Set token kind and
+      // identifier information appropriately.
+      StringRef WithoutQuotes(Result.getLiteralData(), Result.getLength());
+      WithoutQuotes = WithoutQuotes.substr(1, WithoutQuotes.size() - 2);
+      Result.setIdentifierInfo(&Identifiers.get(WithoutQuotes));
+      Result.setKind(tok::identifier);
+    }
     ModuleImportPath.push_back(std::make_pair(Result.getIdentifierInfo(),
----------------
Richard Smith wrote:
> You should check for, and reject, a ud-suffix here. Should we allow implicit string literal concatenation here?
Same as above.

================
Comment at: test/Modules/Inputs/declare-use/h.h:4
@@ -3,3 +3,3 @@
 #include "c.h"
-#include "d.h" // expected-error {{use of a module not declared used}}
+#include "d.h" // expected-error {{does not depend on a module exporting:}}
 #include "h1.h"
----------------
Richard Smith wrote:
> Please check the right values are substituted into this diagnostic in at least one test.
Done (in declare-use1.cpp as that seemed the most fitting).


http://llvm-reviews.chandlerc.com/D2024