[cfe-commits] r42456 - in /cfe/trunk: Lex/IdentifierTable.cpp Parse/ParseObjc.cpp include/clang/Lex/IdentifierTable.h

[Chris Lattner]

On Sep 28, 2007, at 4:39 PM, Steve Naroff wrote:
> Author: snaroff
> Date: Fri Sep 28 18:39:26 2007
> New Revision: 42456

> Add some comments to MultiKeywordSelector, make all methods  
> private, add a friend, move some methods around.

Cool, some implementation comments:


> +IdentifierInfo *Selector::getIdentifierInfoForSlot(unsigned  
> argIndex) {
> +  IdentifierInfo *II = getAsIdentifierInfo();
> +  if (II) {
> +    assert(((argIndex == 0) || (argIndex == 1)) && "illegal  
> keyword index");
> +    return II;
> +  }

This can be:

> +  if (IdentifierInfo *II = getAsIdentifierInfo()) {
> +    assert(argIndex < 2 && "illegal keyword index");
> +    return II;
> +  }

However, is argIndex == 1 really valid?  If not "assert(argIndex ==  
0" is better.


> +char *MultiKeywordSelector::getName(llvm::SmallVectorImpl<char>  
> &methodName) {
> +  methodName[0] = '\0';
> +  keyword_iterator KeyIter = keyword_begin();
> +  for (unsigned int i = 0; i < NumArgs; i++) {
> +    if (KeyIter[i]) {

Instead of subscripting KeyIter[i], please increment the iterator,  
giving you something like this:

> +  for (unsigned int i = 0; i < NumArgs; ++i, ++KeyIter) {
> +    if (const IdentifierInfo *II = *KeyIter) {
  +       use (II)

This will allow the implementation of keyword_iterator to switch from  
random access iterator to forward iterator if we ever needed to in  
the future, and is more natural.

> +char *Selector::getName(llvm::SmallVectorImpl<char> &methodName) {
> +  methodName[0] = '\0';

This assignment accesses past the end of methodName, please remove it.

> +  IdentifierInfo *II = getAsIdentifierInfo();
> +  if (II) {

This can be:

> +  if (IdentifierInfo *II = getAsIdentifierInfo()) {

Also, this seems like it should be in IdentifierTable.cpp, as I  
mentioned before.

-Chris