txExpandedName::init, XMLUtils::isValidQName string do

VERIFIED FIXED in mozilla1.4alpha

Status

()

Core
XSLT
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Axel Hecht, Assigned: Axel Hecht)

Tracking

Trunk
mozilla1.4alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

15 years ago
txExpandedName::init should share code with isValidQName to iterate over the
string only once, and cheap.
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

15 years ago
Created attachment 113520 [details] [diff] [review]
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
(Assignee)

Updated

15 years ago
Attachment #113520 - Flags: superreview?(peterv)
Attachment #113520 - Flags: review?(bugmail)
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.
If you correct those I'll sr this.
(Assignee)

Comment 4

15 years ago
Created attachment 113798 [details] [diff] [review]
fix comments by peterv
(Assignee)

Updated

15 years ago
Attachment #113520 - Flags: superreview?(peterv)
Attachment #113520 - Flags: review?(bugmail)
(Assignee)

Updated

15 years ago
Attachment #113798 - Flags: superreview?(peterv)
Attachment #113798 - Flags: review?(bugmail)
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.
Attachment #113798 - Flags: superreview?(peterv) → superreview+
(Assignee)

Comment 6

15 years ago
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 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
Attachment #113798 - Flags: review?(bugmail) → review+
(Assignee)

Updated

15 years ago
Target Milestone: --- → mozilla1.4alpha
(Assignee)

Comment 8

15 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

15 years ago
mass verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.