There are still a few llvm::ArrayRefs and llvm::StringRefs in the patch left.
re VariantValue:
Is it necessary to introduce the is()/get() magic and a union instead of
using Casting.h-like casts? Do you expect it to significantly help with
readability?
================
Comment at: lib/ASTMatchers/Dynamic/VariantValue.cpp:60-61
@@ +59,4 @@
+void VariantValue::setImpl(const std::string &NewValue) {
+ Type = VT_String;
+ Value.String = new std::string(NewValue);
+}
----------------
Samuel Benzaquen wrote:
> Dmitri Gribenko wrote:
> > What if someone calls setImpl() repeatedly? Do we just leak old values?
> >
> I could call reset() from all the setImpl() overloads. Right now it is called
> once from the place where setImpl() is called.
No need to make the functionality more complex than it is now. `assert(Type ==
VT_Nothing)` here and in the other setImpl() overload should catch the issue.
================
Comment at: include/clang/ASTMatchers/Dynamic/Parser.h:42-43
@@ +41,4 @@
+
+/// \brief Matcher expression parser.
+class Parser {
+public:
----------------
Samuel Benzaquen wrote:
> Dmitri Gribenko wrote:
> > This class just holds a few static member functions. Is the class really
> > needed here?
> It is not really needed. I could take out the class / rename as namespace.
> The same applies to Registry, I guess.
Or you could take advantage of 'Parser' being a real class: if it is
appropriate, you could move 'TokenProcessor' and 'Diagnostics' to be class
members.
http://llvm-reviews.chandlerc.com/D714
BRANCH
svn
ARCANIST PROJECT
clang
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
