[PATCH] Generic Lambdas: A first step

James Dennett jdennett at google.com
Thu Jul 18 14:28:34 PDT 2013


  I'm happy to see patches in this area -- thanks!

  I've not looked in detail at the implementation, but I've a small selection of comments/questions on the documentation/formatting.


================
Comment at: include/clang/AST/DeclCXX.h:551
@@ +550,3 @@
+
+    /// \brief The Lambda call method
+    //CXXMethodDecl *CallOperator;
----------------
Maybe change the Doxygen comment to _not_ be a Doxygen comment while the declaration that it describes is commented out (or, better, remove those changes from this patch).


================
Comment at: include/clang/AST/DeclCXX.h:995
@@ -985,1 +994,3 @@
 
+  /// \brief Determine whether this class describes a Generic 
+  /// lambda function object.
----------------
Generic -> generic ?


================
Comment at: include/clang/AST/DeclCXX.h:996
@@ +995,3 @@
+  /// \brief Determine whether this class describes a Generic 
+  /// lambda function object.
+  bool isGenericLambda() const; 
----------------
Maybe add a short paragraph to say what a "generic" lambda object is -- one whose operator() is a template?

================
Comment at: include/clang/AST/DeclCXX.h:1004
@@ +1003,3 @@
+  /// \brief Retrieve the lambda static invoker, the address of which
+  ///  is returned by the conversion operator, and the body of which
+  ///  is forwarded to the lambda call operator. 
----------------
Nit: For consistency with most comments, I'd avoid indenting the continuation lines
here.

================
Comment at: include/clang/AST/DeclCXX.h:1008
@@ +1007,3 @@
+
+  /// \brief Retrieve the generic lambda's template parameter list
+  TemplateParameterList* getGenericLambdaTemplateParameterList() const;
----------------
Should this also say
  \pre this->isGenericLambda()
or does it return null or an empty list on non-generic lambdas?

================
Comment at: include/clang/AST/ExprCXX.h:1583
@@ +1582,3 @@
+
+  /// \brief Whether this is a generic lambda
+  bool isGenericLambda() const { return !!getTemplateParameterList(); }
----------------
Nit: missing trailing period.

================
Comment at: include/clang/AST/ExprCXX.h:1580
@@ +1579,3 @@
+  /// \brief Retrieve the template parameter list associated with this
+  /// generic lambda expression. 
+  TemplateParameterList *getTemplateParameterList() const;
----------------
Should we document that this returns null if this isn't a generic lambda expression?

================
Comment at: include/clang/Sema/ScopeInfo.h:622
@@ +621,3 @@
+  /// that can be used to construct the generic lambda's template
+  /// parameter list, during initial AST construction.
+  SmallVector<TemplateTypeParmDecl*, 4> AutoTemplateParams;
----------------
The \brief is on the long side of brief ;)

================
Comment at: include/clang/Sema/Sema.h:4364
@@ -4354,1 +4363,3 @@
 
+  /// ActOnLambdaAutoParameter - Invoked when an auto parameter is parsed
+  /// in a lambda's parameter declaration clause.
----------------
s/ActOnLambdaAutoParameter -/\brief/

================
Comment at: include/clang/Sema/Sema.h:4382
@@ -4369,1 +4381,3 @@
+  void buildLambdaScope(sema::LambdaScopeInfo *LSI, 
+                                          CXXMethodDecl *CallOperator,
                                           SourceRange IntroducerRange,
----------------
Broken indentation?

================
Comment at: include/clang/Sema/Sema.h:5755
@@ +5754,3 @@
+  TypeSourceInfo* SubstAutoTypeSourceInfo(TypeSourceInfo *TypeWithAuto, 
+                                   QualType Replacement);
+
----------------
Broken indentation?

================
Comment at: include/clang/Sema/Sema.h:5753
@@ +5752,3 @@
+  QualType SubstAutoType(QualType TypeWithAuto, QualType Replacement);
+  /// \brief Substitute Replacement for Auto in TypeWithAuto
+  TypeSourceInfo* SubstAutoTypeSourceInfo(TypeSourceInfo *TypeWithAuto, 
----------------
Nit: ideally, "for \p Auto in \p TypeWithAuto."

================
Comment at: include/clang/Sema/SemaLambda.h:11
@@ +10,3 @@
+// This file provides some common utility functions for processing
+// Lambdas.
+//
----------------
Can I persuade you to do this in Doxygen style, as

\file
\brief Provides common utility functions for processing Lambdas.

(with leading triple quotes)?

Fine if not, I'm slowly working to update all of these and it'll get caught eventually.



http://llvm-reviews.chandlerc.com/D1174



More information about the cfe-commits mailing list