[cfe-dev] Adding more info to builtin source types.

[Douglas Gregor]

Hello Enea,

On Jan 16, 2010, at 6:05 AM, Enea Zaffanella wrote:

> Douglas Gregor wrote:
>
> [...]
>
>> I actually like the idea of using 3 bits to say which of type/ 
>> signedness/width were specified in the type, then just making sure  
>> that the SourceLocation points to something that makes sense... the  
>> type specifier location if it's there, otherwise the signedness or  
>> width location. That gives us a lot more information in the AST  
>> without costing much at all.
>
> [...]
>
>> Ah, okay. Let's not worry about the source range; a single *useful*  
>> location is good enough.
>
> Here is a revised patch.
>
> We store a single location for all builtin types where you cannot  
> write a width/sign specifier (void, char16, etc.).
>
> For the others builtins, we have additional 4 bytes that are used to  
> store a total of 10 bits (instead of the 3 mentioned above): we  
> found that out initial analysis was incomplete in that we were not  
> considering, e.g., the possibility of having a 'mode' attribute  
> specifying the width of the builtin. Hence now we store:
> - the TST (5 bits), TSW (2 bits) and TSS (2 bits) fields
>   **before** these are modified by the semantic analysis done
>   in DeclSpec::Finish();
> - in the 10th bit, whether or not the ModeAttr was given
>   **before** this is taken away by TakeAttributes().

Okay, great. I committed your patch here:

	http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20100118/026437.html

with a couple of tweaks:

Index: include/clang/AST/TypeLoc.h
===================================================================
--- include/clang/AST/TypeLoc.h (revision 93512)
+++ include/clang/AST/TypeLoc.h (working copy)
@@ -16,6 +16,7 @@

  #include "clang/AST/Type.h"
  #include "clang/AST/TemplateBase.h"
+#include "clang/Parse/DeclSpec.h"

This is a layering violation, because the AST library is not supposed  
to depend on the Parse library. Since type specifiers are a basic  
property of the C languages, I've moved the various TSW/TSS/TST types  
off into a new header, clang/Basic/Specifiers.h to break the layering  
violation.

+  bool needsExtraLocalData() const {
+    BuiltinType::Kind bk = getTypePtr()->getKind();
+    return (bk >= BuiltinType::UShort && bk <= BuiltinType::UInt128)
+      || (bk >= BuiltinType::Short && bk <= BuiltinType::LongDouble)
+      || bk == BuiltinType::UChar
+      || bk == BuiltinType::SChar;
+  }

"float", "double", and "long double" don't need any extra data, since  
we'll always have a location for the type specifier (float or double)  
and the type is never implied by either a sign or a width.

+    default:
+      return DeclSpec::TST_unspecified;

We often try to avoid using "default", and prefer to write out the  
cases. That way, when someone adds a new builtin type, they can work  
through the "missing case" warnings to make sure that type is handled  
properly in various places within the compiler.

+        else if (TL.getWrittenWidthSpec() != DeclSpec::TSS_unspecified)
+          // Width spec loc overrides type spec loc (e.g., 'short  
int').
+          TL.setBuiltinLoc(DS.getTypeSpecWidthLoc());
+      }

Typo here; that TSS_unspecified should be TSW_unspecified.

+DeclSpec::TST BuiltinTypeLoc::getWrittenTypeSpec() const {
+  if (needsExtraLocalData())
+    return static_cast<TST>(getWrittenBuiltinSpecs().Type);
+  else {
+    switch (getTypePtr()->getKind()) {
+    case BuiltinType::Void:
+      return DeclSpec::TST_void;
+    case BuiltinType::Bool:
+      return DeclSpec::TST_bool;
+
+    case BuiltinType::Char_U: // Falling through.
+    case BuiltinType::Char16:
+    case BuiltinType::Char32:
+    case BuiltinType::Char_S:
+    case BuiltinType::WChar:
+      return DeclSpec::TST_char;

This doesn't seem right... wchar_t, char16_t, and char32_t are actual  
type specifiers in C++. Why would we return TST_char for them? Please  
look at the commit to see if you disagree with my changes here.

	- Doug