D26202: Refactor KConfigXT

[Kevin Ottens]

ervin added a comment.


  I'm admittedly very late to the party, so mostly pointing out stylistic things which are left. Since we're touching many lines anyway it's a good point in time to get that improved since the original was very foreign in some way.

INLINE COMMENTS

> KCFGXmlParser.cpp:446
> +// TODO: Change the name of the config variable.
> +KCFGXmlParser::KCFGXmlParser(const KConfigXTParameters &cfg, const QString& inputFileName)
> +    : cfg(cfg), mInputFileName(inputFileName)

Space before & not after

> KCFGXmlParser.h:41
> + */
> +class KCFGXmlParser {
> +public:

Please put the opening curly brace on its own line.

As far as the class name goes I'd propose KCfgXmlParser or KConfigXmlParser to make sure we have a consistent capitalization.

> KCFGXmlParser.h:43
> +public:
> +    KCFGXmlParser(const KConfigXTParameters &cfg, const QString& inputFileName);
> +

Please have the space before & and not after.

> KConfigCodeGeneratorBase.cpp:46
> +KConfigCodeGeneratorBase::KConfigCodeGeneratorBase(
> +    const QString& inputFile,
> +    const QString& baseDir,

Space before & not after

> KConfigCodeGeneratorBase.cpp:80
> +{
> +    if (indentLevel >= 4) {
> +        indentLevel += 2;

This is an odd indentation logic, is it to stay close to the original? (which I'd actually support, just trying to understand where that's coming from).

> KConfigCodeGeneratorBase.cpp:129
> +
> +void KConfigCodeGeneratorBase::addHeaders(const QStringList& headerList)
> +{

Space before & not after

> KConfigCodeGeneratorBase.h:47
> +    KConfigCodeGeneratorBase(
> +        const QString& inputFileName,  // The kcfg file 
> +        const QString& baseDir,        // where we should store the generated file

Space before & not after (and following lines as well).

> KConfigCodeGeneratorBase.h:56
> +    // iterates over the header list adding an #include directive.
> +    void addHeaders(const QStringList& header);
> +

Ditto

> KConfigCodeGeneratorBase.h:65
> +    // Add the correct amount of whitespace in the code.
> +    QString whitespace();
> +

This screams to be const

> KConfigCodeGeneratorBase.h:100
> +
> +    QString inputFile; // the base file name, input file is based on this.
> +

I'd tend to put those private and provide only protected const getters. Just to make sure we don't temper with them by mistake, those instances are in control of the base class after all.

Also I wonder if it wouldn't be the right opportunity to give those members a "m_" prefix? It's the more common style in KF I think.

> KConfigCommonStructs.h:126
> +
> +// TODO: Move those methods
> +QString enumName(const QString &n);

Hell yes, that's will be a nice second step.

> KConfigCommonStructs.h:172
> +QString newItem(
> +    const CfgEntry* entry, 
> +    const QString &key,

Space before * not after

> KConfigHeaderGenerator.cpp:39
> +KConfigHeaderGenerator::KConfigHeaderGenerator(
> +    const QString& inputFile,
> +    const QString& baseDir,

Ditto

> KConfigHeaderGenerator.cpp:382
> +
> +void KConfigHeaderGenerator::createProperties(const CfgEntry *entry, const QString& returnType)
> +{

Ditto

> KConfigHeaderGenerator.cpp:446
> +
> +void KConfigHeaderGenerator::createGetters(const CfgEntry *entry, const QString& returnType)
> +{

Ditto

> KConfigHeaderGenerator.cpp:476
> +
> +void KConfigHeaderGenerator::createItemAcessors(const CfgEntry *entry, const QString& returnType)
> +{

Ditto

> KConfigHeaderGenerator.h:45
> +    KConfigHeaderGenerator(
> +        const QString& inputFile,
> +        const QString& baseDir,

Well, you know it. ;-)

> KConfigHeaderGenerator.h:72
> +    void createSetters(const CfgEntry *entry);
> +    void createItemAcessors(const CfgEntry *entry, const QString& returnType);
> +    void createGetters(const CfgEntry *entry, const QString& returnType);

Ditto

> KConfigSourceGenerator.cpp:31
> +KConfigSourceGenerator::KConfigSourceGenerator(
> +    const QString& inputFile,
> +    const QString& baseDir,

Ditto

> KConfigSourceGenerator.cpp:314
> +
> +void KConfigSourceGenerator::createNormalEntry(const CfgEntry *entry, const QString& key)
> +{

Ditto

> KConfigSourceGenerator.cpp:343
> +
> +void KConfigSourceGenerator::createIndexedEntry(const CfgEntry *entry, const QString& key)
> +{

Ditto

> KConfigSourceGenerator.h:45
> +    KConfigSourceGenerator(
> +        const QString& inputFile,
> +        const QString& baseDir,

Ditto

> KConfigSourceGenerator.h:73
> +    void createEnums(const CfgEntry *entry);
> +    void createNormalEntry(const CfgEntry *entry, const QString& key);
> +    void createIndexedEntry(const CfgEntry *entry, const QString& key);

Ditto

> KConfigXTParameters.h:27
> +
> +#ifndef KCOFIGXTPARAMETERS_H
> +#define KCOFIGXTPARAMETERS_H

Typo, missing N (in both lines)

> KConfigXTParameters.h:37
> +*/
> +class KConfigXTParameters
> +{

I'd go for "Xt"

> kconfig_compiler.cpp:653
>  
> -// adds as many 'namespace foo {' lines to p_out as
> -// there are namespaces in p_ns
> -void beginNamespaces(const QString &p_ns, QTextStream &p_out)
> +bool hasErrors(KCFGXmlParser &parser, const ParseResult& parseResult, const KConfigXTParameters &cfg)
>  {

Space before & not after

REPOSITORY
  R237 KConfig

BRANCH
  rework_kconfig_compiler

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: davidre, bcooksley, cgiboudeaux, kossebau, 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/20200120/f653dcf7/attachment-0001.html>