[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/

[Douglas Gregor]

On Sep 18, 2009, at 2:08 PM, Chris Lattner wrote:

> 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!

Thanks for the review!

>> +++ 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?

Yes, good catch.

>
>> +++ 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? :)

Would be nice.

> 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?

With the ResultSet -> ResultBuilder renaming below, the name Result  
starts to make sense again.

>> +    /// \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? :)

At least until I find something better, yes. This structure keeps  
track of what names we've seen in what scopes, so that we can properly  
determine when a name is hiding another name. Within Sema, we compute  
this information on-the-fly using Scopes, IdentifierInfo chains,  
DeclContexts, etc. There may still be a way to leverage that  
information here.

>> +  /// \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?

Yes. Sorry 'bout the confusion.

>> +  /// \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?

I originally thought that the Result-based interface would be weak  
enough that consumers might want to tweak  
CodeCompleteMemberReferenceExpr et al, but Result is looking fairly  
useful and, with so many hooks in CodeCompleteConsumer, I doubt anyone  
will actually want to mess with them. Time for a refactoring!

	- Doug