191856 - txExpandedName::init, XMLUtils::isValidQName string do

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Axel Hecht]

txExpandedName::init should share code with isValidQName to iterate over the
string only once, and cheap.

Comment 1

[Axel Hecht]

Attachment: fix string code for parsing QNames in XMLUtils

a mere charsink didn't do it, as I need the iterator for colon and copy_string
drops the information which fragment I'm in.
Everything else is pretty simple

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 113520 [details] [diff] [review]
fix string code for parsing QNames in XMLUtils

>+/**
>+ * Helper class for checkin and partioning of QNames

Checking

>+ */
>+class txQNameParser
>+{
>+public:
>+    enum QResult {
>+        eBrokenName,
>+        eOneName,
>+        eTwoNames};

Closing brace on its own line please.

>+    QResult parse(const nsAString::const_iterator& aStart,
>+                  const nsAString::const_iterator& aEnd);
>+
>+    enum {
>+        eInitial,
>+        ePrefixNC,
>+        eColon,
>+        eNameNC,
>+        eBroken} mState;

Closing brace on its own line please.

>+    nsAString::const_iterator mColon;
>+};
>+
>+txQNameParser::QResult
>+txQNameParser::parse(const nsAString::const_iterator& aStart,
>+                     const nsAString::const_iterator& aEnd)
>+{
>+    nsAString::const_iterator chunk(aStart);
>+    mState = eInitial;
>+
>+    PRUint32 size, i;

Don't you have to initialize size to 0?

>+    for ( ; chunk != aEnd; chunk.advance(size)) {
>+        const PRUnichar* buf = chunk.get();
>+        size = chunk.size_forward();
>+
>+        // fragment at 'buf' is 'size' characters long
>+        for (i=0; i<size; i++) {

Add spaces around operators and use ++i

Looks good but I still need to review the parsing itself.

Comment 3

[Peter Van der Beken [:peterv]]

If you correct those I'll sr this.

Comment 4

[Axel Hecht]

Attachment: fix comments by peterv

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 113798 [details] [diff] [review]
fix comments by peterv

I prefer cases in a switch and one-line if's to have braces too but that's just
personal preference. Up to you.

Comment 6

[Axel Hecht]

did the ifs, but not the cases. I insert braces into cases if I have local vars
which this patch does not. nah, don't be consistent.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 113798 [details] [diff] [review]
fix comments by peterv

nit: mState along with its enum could be private.

You might want to add an empty |case eBroken:| or |default:| to the switch, i
think some compiler warns when you're switching on an enum but don't show all
cases.

r=sicking

Comment 8

[Axel Hecht]

checked in

Comment 9

[Axel Hecht]

mass verifying