D26202: WIP: Refactor KConfigXT

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kconfigcompiler_test.cpp:129
> -    // use split('\n') to avoid
> -    // the whole output shown inline
> -    QCOMPARE(content.split('\n'), contentRef.split('\n'));

Why did you remove this? It's just a more user-friendly version of the QVERIFY on the next line, so it can't possibly have failed while the next line passes...

> kconfigcompiler_test.cpp:180
> +        QFile diffFile(oldFile + QStringLiteral(".diff"));
> +        diffFile.open(QIODevice::WriteOnly);
> +

QVERIFY(...)

> kconfigcompiler_test.cpp:183
> +        QTextStream writer(&diffFile);
> +        writer << out;
> +        diffFile.close();

diffFile.write(out); would be simpler than going via QTextStream

> kconfigcompiler_test.cpp:184
> +        writer << out;
> +        diffFile.close();
>      }

not needed, the dtor closes

> KCFGXmlParser.cpp:20
> +
> +// TODO: Remove those global.
> +extern QStringList allNames;

Yes ;)

> KCFGXmlParser.h:1
> +#ifndef KCFGXMLPARSER_H
> +#define KCFGXMLPARSER_H

missing license header

(repeats)

> KCFGXmlParser.h:15
> +    void start();
> +    QString errorString();
> +    ParseResult getParseResult();

... const;

> KCFGXmlParser.h:16
> +    QString errorString();
> +    ParseResult getParseResult();
> +

... const;

> KCFGXmlParser.h:28
> +    // Those are the Entries in the Xml, that represent a parameter within the <group> </group> tag.
> +    void readParameterFromEntry(CfgEntry &entry, const QDomElement &element);
> +    bool hasDefaultCode(CfgEntry &entry, const QDomElement &element);

(to pick on one random method as an example) : this is only used internally by the class, it should be made private. Please do this for all methods that are not used from outside the class. It will make it much clearer what's the actual API of the class.

> KCFGXmlParser.h:34
> +
> +    // TODO: Fix this function later.
> +    CfgEntry *parseEntry(const QString &group, const QDomElement &element);

how, why, when?

> KCFGXmlParser.h:45
> +#endif
> \ No newline at end of file


should be fixed

> KConfigCodeGenerator.h:34
> +
> +/* This class manages the base of writting a C - Based code */
> +class KConfigCodeGenerator {

writing takes a single 't'

> KConfigCodeGenerator.h:35
> +/* This class manages the base of writting a C - Based code */
> +class KConfigCodeGenerator {
> +public:

Maybe call it KConfigCodeGeneratorBase ?

It wasn't obvious to me initially that it was "just" a base class.

> KConfigCodeGenerator.h:37
> +public:
> +    enum ScopeFinalizer {None, Semicollon};
> +

Semicolon takes a single 'l'

> KConfigCodeGenerator.h:91
> +
> +public:
> +    int indentLevel = 0;

urgh, a public variable

> KConfigCommonStructs.h:14
> +
> +// TODO: Remove Signal Arguments for `Param`
> +struct Param

done already?

> KConfigHeaderGenerator.cpp:52
> +
> +    for (auto *entry : parseResult.entries) {
> +        const QString n = entry->name;

qAsConst()

> KConfigHeaderGenerator.cpp:140
> +    stream << endl;
> +    // HACK: Original files ended with two last spaces, add them.
> +    stream << endl;

s/spaces/newlines/

> KConfigHeaderGenerator.cpp:214
> +{
> +
> +}

nothing here?

> KConfigHeaderGenerator.h:73
> +#endif
> \ No newline at end of file


I thought most editors took care of that, these days...

> KConfigSourceGenerator.cpp:40
> +{
> +    qDebug() << "Source Includes" << cfg.sourceIncludes;
> +    

remove (or switch to qCDebug)

> KConfigSourceGenerator.h:46
> +
> +    // Those are fairly self contained functions.
> +    void createHeaders();

Do they all need to be public?

> kconfig_compiler.cpp:745
>  
> -    if (!codegenFilename.endsWith(QLatin1String(".kcfgc"))) {
> -        cerr << "Codegen options file must have extension .kcfgc" << endl;
> -        return 1;
> -    }
> -    QString baseName = QFileInfo(codegenFilename).fileName();
> -    baseName = baseName.left(baseName.length() - 6);
> +    KConfigXTParameters cfg = KConfigXTParameters(codegenFilename);
>  

it's shorter and simpler to write KConfigXTParameters cfg(codegenFilename);

> kconfig_compiler.cpp:753
>  
> -    QFile input(inputFilename);
> +    ParseResult parseResult = xmlParser.getParseResult();
>  

const ... ?

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D26202

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200112/ad3125cb/attachment-0001.html>