[cfe-commits] r82166 - in /cfe/trunk: include/clang/Basic/ include/clang/Lex/ include/clang/Parse/ include/clang/Sema/ lib/Basic/ lib/Lex/ lib/Parse/ lib/Sema/ test/ test/CodeCompletion/ tools/clang-cc/

[Chris Lattner]

On Sep 17, 2009, at 2:32 PM, Douglas Gregor wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=82166&view=rev
> Log:
> Initial implementation of a code-completion interface in Clang. In

Woot!

> +++ cfe/trunk/lib/Lex/Lexer.cpp Thu Sep 17 16:32:03 2009
> @@ -1309,6 +1310,18 @@
>     return true;  // Have a token.
>   }
>
> +  if (IsEofCodeCompletion) {
> +    // We're at the end of the file, but we've been asked to  
> conside the
> +    // end of the file to be a code-completion token. Return the
> +    // code-completion token.
> +    Result.startToken();
> +    FormTokenWithChars(Result, CurPtr, tok::code_completion);
> +
> +    // Only do the eof -> code_completion translation once.
> +    IsEofCodeCompletion = false;
> +    return true;
> +  }

Can this be moved after the "lexing raw mode" case, so that raw mode  
lexing doesn't pay the cost of an extra check?


> +++ cfe/trunk/include/clang/Sema/CodeCompleteConsumer.h Thu Sep 17  
> 16:32:03 2009
> +  /// \brief Captures a result of code completion.
> +  struct Result {
> +    /// \brief Describes the kind of result generated.
> +    enum ResultKind {
> +      RK_Declaration = 0, //< Refers to a declaration
> +      RK_Keyword,        //< Refers to a keyword or symbol.
> +    };
> +
> +    /// \brief The kind of result stored here.
> +    ResultKind Kind;
> +
> +    union {
> +      /// \brief When Kind == RK_Declaration, the declaration we  
> are referring
> +      /// to.
> +      NamedDecl *Declaration;
> +
> +      /// \brief When Kind == RK_Keyword, the string representing  
> the keyword
> +      /// or symbol's spelling.
> +      const char *Keyword;
> +    };

Wouldn't it be nice if PointerIntPair<> fell back to pair<void*,int>  
if the pointer had no extra bits? :)

Should a Result have an optional "extra information" flag or do you  
expect informational stuff to be derived from the decl in all cases?   
i.e. not from sema info?

> +  /// \brief A container of code-completion results.
> +  class ResultSet {

The naming here is a bit strange.  The "Result" of a code completion  
event is a "ResultSet", and a single entry in the result is a  
"Result".  How about naming the classes "ResultEntry" and "Result" or  
something?

> +    /// \brief The actual results we have found.
> +    std::vector<Result> Results;
> +
> +    /// \brief A mapping from declaration names to the declarations  
> that have
> +    /// this name within a particular scope and their index within  
> the list of
> +    /// results.
> +    typedef std::multimap<DeclarationName,
> +                          std::pair<NamedDecl *, unsigned> >  
> ShadowMap;
> +
> +    /// \brief A list of shadow maps, which is used to model name  
> hiding at
> +    /// different levels of, e.g., the inheritance hierarchy.
> +    std::list<ShadowMap> ShadowMaps;

This is a very heavy-weight structure.  I haven't read this in detail  
to understand why you need this, but... do you really need this? :)

> +
> +  public:
> +    typedef std::vector<Result>::iterator iterator;
> +    iterator begin() { return Results.begin(); }
> +    iterator end() { return Results.end(); }

How about just exposing const_iterator instead of iterator?  Do you  
expect clients to want to mutate this?

> +    Result *data() { return Results.empty()? 0 : &Results.front(); }

likewise, const Result*?

> +    /// \brief Add a new result to this result set (if it isn't  
> already in one
> +    /// of the shadow maps), or replace an existing result (for,  
> e.g., a
> +    /// redeclaration).
> +    void MaybeAddResult(Result R);
> +
> +    /// \brief Enter into a new scope.
> +    void EnterNewScope();
> +
> +    /// \brief Exit from the current scope.
> +    void ExitScope();

This style of API seems strange to me: it seems that you'd want to  
pass a fully formed "ResultSet" from Sema into an implementation of  
CodeCompleteConsumer, but ResultSet has a bunch of "builder" APIs.  
Would it make sense to split this out into a CCBuilder thing?

> +  /// \brief Process the finalized code-completion results.
> +  virtual void ProcessCodeCompleteResults(Result *Results,
> +                                          unsigned NumResults) { }

Oh, I see, Sema only gets the array of results.  Given this, should  
"ResultSet" be renamed "ResultBuilder" and moved out of the public  
header file into a private header in the Sema directory?

> +  /// \brief Code completion for a member access expression, e.g.,  
> "x->" or
> +  /// "x.".
> +  ///
> +  /// \param S is the scope in which code-completion occurs.
> +  ///
> +  /// \param BaseType is the type whose members are being accessed.
> +  ///
> +  /// \param IsArrow whether this member referenced was written  
> with an
> +  /// arrow ("->") or a period (".").
> +  virtual void CodeCompleteMemberReferenceExpr(Scope *S, QualType  
> BaseType,
> +                                               bool IsArrow);
> +
> +  /// \brief Code completion for a qualified-id, e.g., "std::"
> +  ///
> +  /// \param S the scope in which the nested-name-specifier occurs.
> +  ///
> +  /// \param NNS the nested-name-specifier before the code- 
> completion location.
> +  ///
> +  /// \param EnteringContext whether the parser will be entering  
> the scope of
> +  /// the qualified-id.
> +  virtual void CodeCompleteQualifiedId(Scope *S,  
> NestedNameSpecifier *NNS,
> +                                       bool EnteringContext);
> +  //@}

Why do these need to be in the public CodeCompleteConsumer interface?   
Doesn't Sema handle all the details of this?  Are implementations of  
consumers expected to implement these?

> +++ cfe/trunk/lib/Sema/CodeCompleteConsumer.cpp Thu Sep 17 16:32:03  
> 2009
> @@ -0,0 +1,314 @@
> +//===---- CodeCompleteConsumer.h - Code Completion Interface ----*-  
> C++ -*-===//

Please update this line

> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open  
> Source
> +// License. See LICENSE.TXT for details.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +//
> +//  This file implements the CodeCompleteConsumer class.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +#include "clang/Sema/CodeCompleteConsumer.h"
> +#include "clang/Lex/Preprocessor.h"
> +#include "Sema.h"
> +#include "llvm/ADT/STLExtras.h"
> +#include "llvm/Support/Compiler.h"

No need to use VISIBILITY_HIDDEN for stuff in anonymous namespaces  
anymore.  We've decided gcc 4.0 doesn't matter for performance anymore.

> +#include "llvm/Support/raw_ostream.h"
> +#include <algorithm>
> +#include <string.h>

<cstring>

Overall, very nice!

-Chris