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

> tcanabrava wrote in kconfigcompiler_test.cpp:129
> the text was really not friendly, it was a big blob of diff content pasted on screen. Considering that the next line will also fail I replaced this by saving the diff file on disk, this way we can actually look at the file that failed and take the time to understand the error.

OK, this needs a hint for the person debugging regressions then. Something like

  QVERIFY2(content == contentRef, "Failure, see foo.diff");

> dfaure wrote in kconfigcompiler_test.cpp:180
> QVERIFY(...)

I meant  QVERIFY2(diffFile.open(...), ...).
No need to make a separate call to isOpen().

> KCFGXmlParser.h:47
> +private:
> +    // creates a `somethignChanged` signal for every property
> +    void createChangedSignal(CfgEntry &readEntry);

typo ("something")

> KConfigCodeGeneratorBase.cpp:2
> +/* This file is part of the KDE libraries
> +    Copyright (C) 2020 Tomaz Cananbrava (tcanabrava at kde.org)
> +

Is this really entirely new code?

Please propagate the copyright of the file you copied this from.

I can agree that .h files are brand new, but not .cpp files

> tcanabrava wrote in KConfigCommonStructs.h:14
> No, there's a struct SignalArguments and a struct Param that are  basically the same thing. A name and a Type. Still in the TODO.

Oh. I thought it meant "remove SignalArguments members from Param".

Can you at least clarify the comment by saying "TODO merge Param and SignalArguments" or (in front of SignalArguments) "TODO use Param instead"?

> tcanabrava wrote in KConfigHeaderGenerator.h:73
> Kate / KDevelop here. I'm manually adding them.

I'm no expert but I'm sure there's an option for this somewhere....

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

There are still some qDebugs, please do a patch-wide search.

> KConfigSourceGenerator.cpp:224
> +    if (parseResult.cfgFileNameArg) {
> +        if (! cfg.forceStringFilename) {
> +            stream << " KSharedConfig::Ptr config";

no space after '!'

> KConfigSourceGenerator.cpp:391
> +}
> +void KConfigSourceGenerator::doConstructor() {
> +

newline before '{'

> kconfig_compiler.cpp:762
> +    const QString baseName = inputFilename.mid(0, inputFilename.size()-5);
> +    qDebug() << "Base name calculed" << baseName;
>  

^

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/20200113/fb813900/attachment-0001.html>