[Webkit-unassigned] [Bug 42976] Implement IDBKeyPath parser.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42976





--- Comment #18 from Marcus Bulach <bulach at chromium.org>  2010-08-05 02:29:48 PST ---
replying Jeremy's comments from https://bugs.webkit.org/show_bug.cgi?id=43276
the major change is that I removed the class and instead there's a function that returns a vector of IDBKeyPathElements.
plus some other clean ups. more below.
another look please?


(In reply to comment #11)
> (From update of attachment 63313 [details])
> WebCore/storage/IDBKeyPath.cpp:101
>  +              if (token == TokIdentifier)
> Use a switch?
hmm, it doesn't seem it'd make it any more readable.

> 
> WebCore/storage/IDBKeyPath.cpp:46
>  +      , m_currentToken(0)
> Use the enum value
this is gone.

> 
> WebCore/storage/IDBKeyPath.cpp:58
>  +      return m_currentToken < m_tokens.size();
> I guess I'd suggest just converting over all this hasNext/next stuff to just returning the raw vector.  I don't feel strongly, but I just don't see much of a point of creating this interface.

removed the class, converted to a single function that returns a vector of elements.

> 
> WebCore/storage/IDBKeyPath.cpp:70
>  +          return "";
> Maybe you should just let it crash?  I.e. not do bounds checking?
obsolete.

> 
> WebCore/storage/IDBKeyPath.cpp:90
>  +      return m_parserError;
> Maybe all the methods should be ASSERTing that a parse error never happened?
the function now returns an error, it may be better to let the caller decide what to do with it?

> 
> WebCore/storage/IDBKeyPath.cpp:103
>  +              else if (token == TokLBracket)
> I'd lean towards spelling these all the way out.
Done.

> 
> WebCore/storage/IDBKeyPath.cpp:116
>  +              Token idbToken;
> these 2 token variable names are confusing.  The idb doesn't make it any less so.  Maybe 'outputToken' or something?
There's now a TokenType (with the names spelled out as above), and IDBKeyPathElement, let me know if it's any clearer.

> 
> 
> 
> general note: you should talk to the guy who does fuzzers (skylined..?) and see if we can get some coverage on this
indeed! once we agree on the implementation, I'll add more people to it.

> 
> also, could we just do this more simply?  for example, in python pseudo code I could do this:
> 
> tokens = []
> if (path == ""):
>   return []
> for (component in path.split(".")):
>   if (component ~= /^([a-zA-Z0-9_]+)\[([0-9]+)\]$/):
>     tokens.append($1, $2)
>   else if (component ~= /^([a-zA-Z0-9_]+)$/):
>     tokens.append($1, null)
>   else:
>     return error
> return tokens
> And that'd be about it.  Do we have any sort of regex support build into WebKit already?  The split would be pretty easy to do as well.  I'm just concerned that we're jumping the gun with all of this complexity.
> 
> Anyhow...

I don't think this would be much simpler.. We'd still need to get the numbers out of the indices.
There's the n-ary array case "foo[3][4]", etc.. seems that this single-pass parser might be simpler in the end?

> 
> 
> WebCore/storage/IDBKeyPath.cpp:145
>  +              ASSERT(token.type == TokNumber);
> this seems mildly redundant, but I'm ok with it
> 
> WebCore/storage/IDBKeyPath.cpp:163
>  +                  state = Array;
> but this is an invalid state!

not really. it's perfectly possible to have n-ary array, i.e., "foo[3][9][5].age". ValidKeyPath1 test exercises this.

> 
> WebCore/storage/IDBKeyPath.cpp:147
>  +              idbToken.hasIndex = true;
> maybe assert it was false?
obsolete.

> 
> WebCore/storage/IDBKeyPath.cpp:190
>  +  
> default:
>   raise error
as above, perhaps the caller should decide?

> 
> WebCore/storage/IDBKeyPath.cpp:99
>  +          case Start : {
> maybe pull this out of the state machine?
done.

> 
> WebCore/storage/IDBKeyPath.cpp:278
>  +          return TokError;
> I'd just do an m_ptr >= m_end first and then return...then no need for an else
done.

> 
> WebCore/storage/IDBKeyPath.cpp:292
>  +      return TokError;
> same again...test the error condition and return rather than putting everythign in a block
done.

> 
> WebCore/storage/IDBKeyPath.cpp:252
>  +      if (m_ptr < m_end && isSafeIdentifierStartCharacter(*m_ptr))
> ditto
done.

> 
> WebCore/storage/IDBKeyPath.h:84
>  +      class Lexer {
> Why does this need to be defined here?
moved to the cpp file.

> 
> WebCore/storage/IDBKeyPath.h:116
>  +          const UChar* m_ptr;
> should this be called m_cursor?
hmm, cursor and idb is a combination already in use..
isn't ptr clear?

> 
> WebCore/storage/IDBKeyPath.h:100
>  +          TokenType next()
> these could all be on one line
removed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.