[PATCH] D44695: [clang-format] Partially revert r322749, replacing array with DenseSet

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

> One way in which DenseSet is better is that it supports StringRefs - we don't 
> have to define hash. Seems like the lack of this override in core LLVM 
> suggests that unordered_set is not commonly used with StringRefs.

Makes sense, thanks.


Repository:
  rC Clang

https://reviews.llvm.org/D44695



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

[PATCH] D44695: [clang-format] Partially revert r322749, replacing array with DenseSet

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

I'm fine with whichever approach we use, but I'll defer to @krasimir here.

(I'm not fully sure why `llvm::DenseSet` is better than `std::unsorted_set`, 
but I'm sure you and @krasimir understand the subtleties better than I do.)


Repository:
  rC Clang

https://reviews.llvm.org/D44695



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

[PATCH] D44695: [clang-format] Partially revert r322749, replacing array with DenseSet

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer accepted this revision.
bkramer added a comment.
This revision is now accepted and ready to land.

I wouldn't say that this is more maintainable, but I'm not the maintainer of 
clang-format.


Repository:
  rC Clang

https://reviews.llvm.org/D44695



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

[PATCH] D44695: [clang-format] Partially revert r322749, replacing array with DenseSet

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir created this revision.
Herald added subscribers: cfe-commits, klimek.

The array + binary_search requires the initializer list of identifiers to be
sorted. I can imagine how this list might change frequently while the support
for ObjectiveC improves (https://reviews.llvm.org/D44632). The array approach 
might have been a premature
optimization too, since in any case computing a hash for every token is trivial
compared to what we do for formatting it.


Repository:
  rC Clang

https://reviews.llvm.org/D44695

Files:
  lib/Format/Format.cpp


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -31,6 +31,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
@@ -1446,8 +1447,7 @@
 private:
   static bool guessIsObjC(const SmallVectorImpl 
,
   const AdditionalKeywords ) {
-// Keep this array sorted, since we are binary searching over it.
-static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
+static const auto *FoundationIdentifiers = new llvm::DenseSet{
 "CGFloat",
 "NSAffineTransform",
 "NSArray",
@@ -1510,9 +1510,8 @@
   FormatTok->isOneOf(tok::numeric_constant, tok::l_square,
  tok::l_brace))) ||
 (FormatTok->Tok.isAnyIdentifier() &&
- std::binary_search(std::begin(FoundationIdentifiers),
-std::end(FoundationIdentifiers),
-FormatTok->TokenText)) ||
+ FoundationIdentifiers->find(FormatTok->TokenText) !=
+ FoundationIdentifiers->end()) ||
 FormatTok->is(TT_ObjCStringLiteral) ||
 FormatTok->isOneOf(Keywords.kw_NS_ENUM, Keywords.kw_NS_OPTIONS,
TT_ObjCBlockLBrace, TT_ObjCBlockLParen,


Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -31,6 +31,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
@@ -1446,8 +1447,7 @@
 private:
   static bool guessIsObjC(const SmallVectorImpl ,
   const AdditionalKeywords ) {
-// Keep this array sorted, since we are binary searching over it.
-static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
+static const auto *FoundationIdentifiers = new llvm::DenseSet{
 "CGFloat",
 "NSAffineTransform",
 "NSArray",
@@ -1510,9 +1510,8 @@
   FormatTok->isOneOf(tok::numeric_constant, tok::l_square,
  tok::l_brace))) ||
 (FormatTok->Tok.isAnyIdentifier() &&
- std::binary_search(std::begin(FoundationIdentifiers),
-std::end(FoundationIdentifiers),
-FormatTok->TokenText)) ||
+ FoundationIdentifiers->find(FormatTok->TokenText) !=
+ FoundationIdentifiers->end()) ||
 FormatTok->is(TT_ObjCStringLiteral) ||
 FormatTok->isOneOf(Keywords.kw_NS_ENUM, Keywords.kw_NS_OPTIONS,
TT_ObjCBlockLBrace, TT_ObjCBlockLParen,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits