[cfe-dev] [PATCH]: wchar_t foo[] = L"bar"

[Doug Gregor]

On Fri, Nov 21, 2008 at 9:11 AM, Roman Divacky <rdivacky at freebsd.org> wrote:
> hi
>
> the $subj is currently rejected by clang because it does not know
> this is allowed by the C standard.. I've come with a simple patch
> that lets this happen. It introduces a new isWideCharType() method
> to ASTContext class (as suggested by Daniel Dunbar) and uses it
> to check if this contstruction is allowed...
>
> the patch:  www.vlakno.cz/~rdivacky/wchar.patch
>
> please review/comment and possibly commit :)

Some comments:

+  /// Detects whether a type is of WChar pointer type
+  bool isWideCharType(QualType Ty) const;

With this addition, we now have two isWideCharTypes: one in
ASTContext, and one in Type. The latter only works in C++, while the
one you've added currently works for C and C++. One of them will have
to be removed.

+bool ASTContext::isWideCharType(QualType Ty) const {
+   return Ty->getCanonicalTypeInternal().getUnqualifiedType() ==
getWCharType();
+}

This will always work in C++ (where wchar_t is a distinct type), but
the fact that it works now in C is due to an error in
ASTContext::getWCharType, which isn't doing the right thing for C:

QualType ASTContext::getWCharType() const {
  if (LangOpts.CPlusPlus)
    return WCharTy;

  // FIXME: In C, shouldn't WCharTy just be a typedef of the target's
  // wide-character type?
  return getFromTargetType(Target.getWCharType());
}

If we fix getWCharType, ASTContext::isWideCharType will no longer
work, since getWCharType would then return a typedef. I think we
should either fix both isWideCharType and getWCharType to do the same
thing, or find a different way around the problem. The second one is
easier for now, because Sema::IsStringLiteralInit is more complicated
than it needs to be:

StringLiteral *Sema::IsStringLiteralInit(Expr *Init, QualType DeclType) {
   const ArrayType *AT = Context.getAsArrayType(DeclType);
-  if (AT && AT->getElementType()->isCharType()) {
+  if (AT && AT->getElementType()->isCharType() ||
+	Context.isWideCharType(AT->getElementType())) {
     return dyn_cast<StringLiteral>(Init);
   }

Why do we bother checking at the type of a string literal is an array
of characters? We know it's an array of characters, because it's a
string literal. So it seems that the checking of the array type could
be removed, and we could just dyn_cast<StringLiteral>(Init). That also
side-steps the issue of isWideCharType (for now).

Oh, and I know you have a test-case in the bug report, but could you
include the new tests as part of the patch? The tests should go into
test/Sema or test/SemaCXX for this kind of change.

Thanks!

  - Doug