[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

2019-01-18 Thread Bruno Ricci via Phabricator via cfe-commits 
riccibruno abandoned this revision.
riccibruno added a comment.

This is not worth doing.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54324/new/

https://reviews.llvm.org/D54324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

2018-11-15 Thread Shafik Yaghmour via Phabricator via cfe-commits 
shafik added inline comments.



Comment at: include/clang/AST/Expr.h:1407
 public:
-  enum CharacterKind {
-Ascii,
-Wide,
-UTF8,
-UTF16,
-UTF32
-  };
+  enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 };
 

riccibruno wrote:
> shafik wrote:
> > Minor comment, does it make sense to covert this to a scoped enum since it 
> > looks like it is being strictly used as a set of values.
> Does it really add anything ?
> It means that instead of writing `CharacterLiteral::UTF32`
> you have to write `CharacterLiteral::CharacterKind::UTF32`
> 
> Seems a bit verbose. But I don't have any strong opinion on this.
It adds the protection against unintended conversions to and from the scoped 
enum, which in general is a good thing. The other benefits is not having the 
enumerators leak out into the surrounding scope but is limited in this case.

It does add verbosity though.


Repository:
  rC Clang

https://reviews.llvm.org/D54324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

2018-11-12 Thread Bruno Ricci via Phabricator via cfe-commits 
riccibruno added inline comments.



Comment at: include/clang/AST/Expr.h:1407
 public:
-  enum CharacterKind {
-Ascii,
-Wide,
-UTF8,
-UTF16,
-UTF32
-  };
+  enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 };
 

shafik wrote:
> Minor comment, does it make sense to covert this to a scoped enum since it 
> looks like it is being strictly used as a set of values.
Does it really add anything ?
It means that instead of writing `CharacterLiteral::UTF32`
you have to write `CharacterLiteral::CharacterKind::UTF32`

Seems a bit verbose. But I don't have any strong opinion on this.


Repository:
  rC Clang

https://reviews.llvm.org/D54324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

2018-11-12 Thread Shafik Yaghmour via Phabricator via cfe-commits 
shafik added inline comments.



Comment at: include/clang/AST/Expr.h:1407
 public:
-  enum CharacterKind {
-Ascii,
-Wide,
-UTF8,
-UTF16,
-UTF32
-  };
+  enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 };
 

Minor comment, does it make sense to covert this to a scoped enum since it 
looks like it is being strictly used as a set of values.


Repository:
  rC Clang

https://reviews.llvm.org/D54324



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits