[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-commits
Subject:    Re: KDE/kdelibs/kdecore/config
From:       Jakub Stachowski <stachowski () hypair ! net>
Date:       2008-04-27 13:15:58
Message-ID: 200804271515.58619.stachowski () hypair ! net
[Download RAW message or body]

Dnia niedziela, 27 kwietnia 2008, Oswald Buddenhagen napisał:
> reviewing it again ...
>
> On Mon, Apr 21, 2008 at 02:01:58PM +0000, Jakub Stachowski wrote:
> > SVN commit 799413 by qbast:
> >
> > Ini parsing optimization - avoid using QByteArray and excessive memory
> > copying.
> >
> > --- trunk/KDE/kdelibs/kdecore/config/kconfigini.cpp #799412:799413
> > @@ -165,17 +174,17 @@
> >              if (groupOptionImmutable)
> >                  entryOptions |= KEntryMap::EntryImmutable;
> >
> >              BufferFragment locale;
> >              BufferFragment rawKey;
> >              int start;
> > -            while ((start = aKey.indexOf('[')) >= 0) {
> > +            while ((start = aKey.lastIndexOf('[')) >= 0) {
> >                  int end = aKey.indexOf(']', start);
> > @@ -188,8 +197,9 @@
> >                                  break;
> >                              case 'd':
> >                                  entryOptions |= KEntryMap::EntryDeleted;
> > -                                aKey =
> > printableToString(aKey.left(start), file, lineNo); -                     
> >           entryMap.setEntry(currentGroup, aKey, QByteArray(),
> > entryOptions); +                                aKey = aKey.left(start);
> > +                                printableToString(&aKey, file, lineNo);
> > +                                entryMap.setEntry(currentGroup,
> > aKey.toByteArray(), QByteArray(), entryOptions); goto next_line;
> >                              default:
> >                                  break;
> > @@ -203,17 +213,16 @@
> >                          goto next_line;
> >                      }
> >
> >                      locale = aKey.mid(start + 1,end - start - 1);
> >                      rawKey = aKey.left(end + 1);
> >                  }
> > -                aKey.remove(start, end-start+1);
> > +                aKey.truncate(start);
> >              }
>
> i *think* you broke the case where the immutability marker comes before
> the locale marker:
>
> Key[$i][de_DE]=foo
>
> care to craft a test?

My changes completely botched rawKey, so line like entry[$i][fr]=test after 
sync would become entry[$i][fr][$i]=test :-(

Attached fix (with unittest) fixes this behaviour by recreating raw key from 
parsed key (without unescaping) +'['+locale+']'. As a bonus it is about ~1% 
faster (due to omitting to printableToString for raw keys).
Do you still see anything suspicious?

["localefix.patch" (text/x-diff)]

Index: tests/kconfigtest.cpp
===================================================================
--- tests/kconfigtest.cpp	(wersja 799070)
+++ tests/kconfigtest.cpp	(kopia robocza)
@@ -773,6 +773,45 @@
     QVERIFY(cg2.isImmutable());
 }
 
+void KConfigTest::testOptionOrder()
+{
+    {
+        QFile file(KStandardDirs::locateLocal("config", "doubleattrtest"));
+        file.open(QIODevice::WriteOnly|QIODevice::Text);
+        QTextStream out(&file);
+        out.setCodec("UTF-8");
+        out << "[group3]" << endl
+            << "entry2=unlocalized" << endl
+            << "entry2[$i][de_DE]=t2" << endl;
+    }
+    KConfig config("doubleattrtest", KConfig::SimpleConfig);
+    config.setLocale("de_DE");
+    KConfigGroup cg3 = config.group("group3");
+    QVERIFY(!cg3.isImmutable());
+    QCOMPARE(cg3.readEntry("entry2",""), QString("t2"));
+    QVERIFY(cg3.isEntryImmutable("entry2"));
+    config.setLocale("C");
+    QCOMPARE(cg3.readEntry("entry2",""), QString("unlocalized"));
+    QVERIFY(!cg3.isEntryImmutable("entry2"));
+    cg3.writeEntry("entry2","modified");
+    config.sync();
+
+    {
+        QList<QByteArray> lines;
+        // this is what the file should look like
+        lines << "[group3]\n" 
+              << "entry2=modified\n"
+              << "entry2[de_DE][$i]=t2\n";
+
+        QFile file(KStandardDirs::locateLocal("config", "doubleattrtest"));
+        file.open(QIODevice::ReadOnly|QIODevice::Text);
+        foreach (const QByteArray& line, lines) {
+            QCOMPARE(line, file.readLine());
+        }
+    }
+}
+
+
 void KConfigTest::testGroupEscape()
 {
     KConfig config("groupescapetest", KConfig::SimpleConfig);
Index: tests/kconfigtest.h
===================================================================
--- tests/kconfigtest.h	(wersja 799070)
+++ tests/kconfigtest.h	(kopia robocza)
@@ -63,6 +63,7 @@
     void testWriteOnSync();
     void testCreateDir();
     void testSharedConfig();
+    void testOptionOrder();
 
     // unrelated
     void testKAboutDataOrganizationDomain();
Index: config/kconfigini.cpp
===================================================================
--- config/kconfigini.cpp	(wersja 801679)
+++ config/kconfigini.cpp	(kopia robocza)
@@ -175,7 +175,6 @@
                 entryOptions |= KEntryMap::EntryImmutable;
 
             BufferFragment locale;
-            BufferFragment rawKey;
             int start;
             while ((start = aKey.lastIndexOf('[')) >= 0) {
                 int end = aKey.indexOf(']', start);
@@ -214,7 +213,6 @@
                     }
 
                     locale = aKey.mid(start + 1,end - start - 1);
-                    rawKey = aKey.left(end + 1);
                 }
                 aKey.truncate(start);
             }
@@ -227,16 +225,17 @@
                 if (locale != currentLocale) {
                     // backward compatibility. C == en_US
                     if (locale.at(0) != 'C' || currentLocale != "en_US") {
-                        if (merging){
+                        if (merging)
                             entryOptions |= KEntryMap::EntryRawKey;
-                            aKey = rawKey; // store as unprocessed key
-                            locale = BufferFragment();
-                        } else
+                        else
                             goto next_line; // skip this entry if we're not merging
                     }
                 }
             } 
 
+            if (!(entryOptions&KEntryMap::EntryRawKey)) 
+                printableToString(&aKey, file, lineNo);
+
             if (options&ParseGlobal)
                 entryOptions |= KEntryMap::EntryGlobal;
             if (bDefault)
@@ -244,7 +243,14 @@
             if (!locale.isNull())
                 entryOptions |= KEntryMap::EntryLocalized;
             printableToString(&line, file, lineNo);
-            entryMap.setEntry(currentGroup, aKey.toByteArray(), line.toByteArray(), entryOptions);
+            if (entryOptions&KEntryMap::EntryRawKey) {
+                QByteArray rawKey;
+                rawKey.reserve(aKey.length()+locale.length()+2);
+                rawKey.append(aKey.toSharedByteArray()).append('[');
+                rawKey.append(locale.toSharedByteArray()).append(']');
+                entryMap.setEntry(currentGroup, rawKey, line.toByteArray(), entryOptions);
+            } else 
+                entryMap.setEntry(currentGroup, aKey.toByteArray(), line.toByteArray(), entryOptions);
         }
 next_line:
         continue;
Index: config/bufferfragment_p.h
===================================================================
--- config/bufferfragment_p.h	(wersja 801679)
+++ config/bufferfragment_p.h	(kopia robocza)
@@ -166,6 +166,12 @@
     QByteArray toByteArray() const {
         return QByteArray(d,len);
     }
+    
+    // this is faster than toByteArray, but returned QByteArray becomes invalid
+    // when buffer for this BufferFragment disappears
+    QByteArray toSharedByteArray() const {
+        return QByteArray::fromRawData(d, len);
+    }
 
 private:
     char* d;


[prev in list] [next in list] [prev in thread] [next in thread]