D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Tomaz Canabrava]

tcanabrava added a comment.


  +1. @cfeck do you think this can land now?

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  named_color_support

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: cblack, broulik, cfeck, kde-frameworks-devel, LeGast00n, michaelh, ngraham, 
bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Tomaz Canabrava]

tcanabrava added inline comments.

INLINE COMMENTS

> kcolorcombo.cpp:244
> +{
> +QList namedColors;
> +for (int colorIndex = 0; colorIndex < colors.count(); colorIndex++) {

namedColors.reserve(colors.size());

> kcolorcombo.cpp:245
> +QList namedColors;
> +for (int colorIndex = 0; colorIndex < colors.count(); colorIndex++) {
> +namedColors.insert(colorIndex, { QString(), colors[colorIndex] });

for(auto color : colors)

> kcolorcombo.cpp:288
> +for (int index = 0; index < STANDARD_PALETTE_SIZE; ++index) {
> +//list += {QString(), standardColor(index)};
> +}

why is there a for running with all code comented out?

> kcolorcombo.cpp:291
> +return list;
>  } else {
>  return d->colorList;

remove the else, just return directly.

> araujoluis wrote in kcolorcombo.h:60
> broulik try a solution in several ways, but it looks like this was the most 
> convenient one found so far.

QPair - no need for a struct.

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: broulik, cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29502: kwidgetsaddons: Add a named colors support in KColorCombo.

[Tomaz Canabrava]

tcanabrava added a comment.


  In D29502#665582 , @cfeck wrote:
  
  > Does the delegate ensure the text is rendered in a color visible over the 
colored background?
  
  
  not yet, I talked to gustavo and he's working in an updated version of the 
patch.
  
  This is the current - not in the diff yet - version:
  F8293330: image.png 

REPOSITORY
  R236 KWidgetsAddons

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

To: araujoluis, tcanabrava, patrickelectric, hindenburg, ngraham
Cc: cfeck, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

Re: D26877: Simplify calls to whitespace() and use it in more places.

[Tomaz Canabrava]

I like indentedStream.


On Sun, 2 Feb 2020 at 11:36 David Faure  wrote:

> dfaure requested changes to this revision.
> dfaure added a comment.
> This revision now requires changes to proceed. View Revision
> 
>
> I don't like it either. It doesn't "read" well.
> Looking at cout or qDebug it's much more common to [the usual stream] <<
> [some modifier] << some more stuff.
>
> Maybe it can be solved with naming though.
> indentedStream() << ...
> ?
>
> *REPOSITORY*
> R237 KConfig
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26877
>
> *To: *tcanabrava, dfaure, ervin
> *Cc: *kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
>

D26877: Simplify calls to whitespace() and use it in more places.

[Tomaz Canabrava]

tcanabrava added a subscriber: ervin.
tcanabrava added a comment.


  I like indentedStream.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, dfaure, ervin
Cc: ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Tomaz Canabrava]

tcanabrava added a comment.


  
  
  > That's why personally I think it's fine to assume people need to opt-in for 
GenerateProperties if they want the feature, it's closely related after all.
  
  I disagree here.
  my application can be a simple QWidget based and have a KConfigXT enabled 
configuration dialog that I want to trigger a save on any edit.
  I don't need properties for that.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26868: Move newItem to private method in KConfigSourceGenerator

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 74600.
tcanabrava added a comment.


  - Fix code style

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26868?vs=74234=74600

BRANCH
  simplify_newEntry

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

AFFECTED FILES
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, ervin, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26868: Move newItem to private method in KConfigSourceGenerator

[Tomaz Canabrava]

tcanabrava added a comment.


  In D26868#602162 , @kossebau wrote:
  
  > In D26868#602150 , @dfaure wrote:
  >
  > > There is indeed a QString overload for concatenating QLatin1String, but 
it will have to be converted char-by-char (from 8 bits to 16 bits), so isn't it 
faster to concatenate QStringLiterals?
  >
  >
  > I have never seen numbers, but any time I asked someone who might have seen 
numbers, they told me: QStringLiteral only for final strings not further 
modified or given to operations. Sadly cannot find references now, but I asked 
often the recent years.
  >  Looking at the code, the `qt_from_latin1` helper method might be as 
efficient on most processors as the `memcpy` is perhaps, and making up for any 
other overhead the QStringLiteral-generated code has, perhaps atomic stuff 
around the generated QString instance is expensive compared to the copy logic?
  >
  > But ready to hear from real Qt experts once more, perhaps with numbers this 
time :)
  
  
  I checked here with a really big xml file and the difference seems 
negligible. ( QStringLiteral 0.0s and QLatin1String 0.0s ) - I'd use 
QStringLiteral just for the sake of consistency across the codebase, tough

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26127: Simplify cppType method: Return Early, Use a Map and Assert.

[Tomaz Canabrava]

tcanabrava abandoned this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26128: Simplify defaultValue method: Return Early, Use Default Initialization, and Assert.

[Tomaz Canabrava]

tcanabrava abandoned this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26126: Simplify param method: Return Early, Use a Map and Assert.

[Tomaz Canabrava]

tcanabrava abandoned this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: dfaure, ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D26130: Simplify return logic

[Tomaz Canabrava]

tcanabrava abandoned this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, patrickelectric, ervin
Cc: ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26129: Remove Iterator based loops for range based loops

[Tomaz Canabrava]

tcanabrava abandoned this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26876: Remove the Enum hack: finish lists with a comma, it's valid c++

[Tomaz Canabrava]

tcanabrava added a comment.


  update / rebased.

INLINE COMMENTS

> ervin wrote in KConfigHeaderGenerator.cpp:252
> I lost track of constness here so I might be wrong, shouldn't this use 
> qAsConst in this context?

yeah.

REPOSITORY
  R237 KConfig

BRANCH
  remove_enum_hack

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

To: tcanabrava, ervin, dfaure, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26876: Remove the Enum hack: finish lists with a comma, it's valid c++

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 74596.
tcanabrava marked an inline comment as done.
tcanabrava added a comment.


  - Use qAsConst

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26876?vs=74253=74596

BRANCH
  remove_enum_hack

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

AFFECTED FILES
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test_signal.h.ref
  src/kconfig_compiler/KConfigHeaderGenerator.cpp

To: tcanabrava, ervin, dfaure, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26368: Add an isImmutable to know if a property is immutable

[Tomaz Canabrava]

tcanabrava added a comment.


  it's a +1 for me. @dfaure and @ervin ?

REPOSITORY
  R237 KConfig

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

To: meven, ervin, #frameworks
Cc: dfaure, tcanabrava, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26368: Add an isImmutable to know if a property is immutable

[Tomaz Canabrava]

tcanabrava added a comment.


  I like it, but considering that this adds a new method, I'd like to see it 
exposed to Qml too o the generated code if GenerateProperties is set to true, 
currently we write this kind of code in Qml:
  
  enabled: !kcm.balooSettings.isImmutable("indexingEnabled")

something in the lines of `settings.indexingEnabledIsImmutable` would be of 
great help.
  
  (I would go further and try to add a class for each property with a 
.isImmutable, .name, .value and so on but this is out of scope and *maybe* only 
for KF6.)

REPOSITORY
  R237 KConfig

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

To: meven, ervin, #frameworks
Cc: dfaure, tcanabrava, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26573: Add missing Import Env Variable

[Tomaz Canabrava]

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:ee0531749057: Add missing Import Env Variable (authored 
by tcanabrava).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26573?vs=74512=74513

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

AFFECTED FILES
  kde-modules/prefix.sh.cmake

To: tcanabrava, mart
Cc: apol, davidedmundson, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D26573: Add missing Import Env Variable

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 74512.
tcanabrava added a comment.


  - Fix Variable

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26573?vs=73237=74512

BRANCH
  add_missing_env_var

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

AFFECTED FILES
  kde-modules/prefix.sh.cmake

To: tcanabrava, mart
Cc: apol, davidedmundson, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D26961: Be more helpfully verbose when we can't start an action

[Tomaz Canabrava]

tcanabrava accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R283 KAuth

BRANCH
  master

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

To: davidedmundson, tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26133: Enable Auto Save

[Tomaz Canabrava]

tcanabrava added a comment.


  I agree with the comments, but I'm a bit lost on how to implement that in 
KCoreConfigSkeleton:
  the isSaveNeeded reads the value of the variable and return if it's different 
from the reference variable. (that I tougth it was a reference *value*, to find 
out that it's a *variable reference*.
  so, if I change my setting via:
  
  Setting::self()->setSomething(5)
  
  the code for this setSomething will not call any KCoreConfigSkeleton code, 
but will just do a:
  
  GeneratedConfig::setSomething(int value) {
  
mSomething = value;
  
  }
  
  this is then handled internally on the KCoreSkeletonItem but I don't really 
understand how.
  anyone has a hint on how I can trigger a signal on it?
  I'm a bit lost.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, GB_2, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D26131: Braces around for, break early.

[Tomaz Canabrava]

tcanabrava added reviewers: ervin, dfaure.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: ervin, patrickelectric, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26867: New class: KConfigTypeInformation

[Tomaz Canabrava]

tcanabrava added reviewers: ervin, dfaure.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26876: Remove the Enum hack: finish lists with a comma, it's valid c++

[Tomaz Canabrava]

tcanabrava added reviewers: ervin, dfaure.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26868: Move newItem to private method in KConfigSourceGenerator

[Tomaz Canabrava]

tcanabrava added reviewers: ervin, dfaure.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26877: Simplify calls to whitespace() and use it in more places.

[Tomaz Canabrava]

tcanabrava added reviewers: dfaure, ervin.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, dfaure, ervin
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26131: Braces around for, break early.

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 74258.
tcanabrava added a comment.


  - use std::any_of

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26131?vs=71913=74258

BRANCH
  arcpatch-D26131

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

AFFECTED FILES
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: ervin, patrickelectric, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D26877: Simplify calls to whitespace() and use it in more places.

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
tcanabrava requested review of this revision.

REVISION SUMMARY
  whitespace now returns a textStream so we don't need to stream() << 
whitespace()
  Use whitespace where we had stream() << " followed by 4 spaces
  
  There are plenty of manual space manipulation on the code that I plan to
  get rid of, but that will have to wait as it will break every single
  testcase.

TEST PLAN
  Run unittests

REPOSITORY
  R237 KConfig

BRANCH
  simplify_whitespace

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

AFFECTED FILES
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26876: Remove the Enum hack: finish lists with a comma, it's valid c++

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
tcanabrava requested review of this revision.

TEST PLAN
  Run tests

REPOSITORY
  R237 KConfig

BRANCH
  remove_enum_hack

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

AFFECTED FILES
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test_signal.h.ref
  src/kconfig_compiler/KConfigHeaderGenerator.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26868: Move newItem to private method in KConfigSourceGenerator

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
tcanabrava requested review of this revision.

REVISION SUMMARY
  This is only used here, should only be exposed here.

TEST PLAN
  Run unittests

REPOSITORY
  R237 KConfig

BRANCH
  simplify_newEntry

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

AFFECTED FILES
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26867: New class: KConfigTypeInformation

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 74231.
tcanabrava added a comment.


  - Fix position of reference on parameter

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26867?vs=74224=74231

BRANCH
  rewrite_maps

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigTypeInformation.cpp
  src/kconfig_compiler/KConfigTypeInformation.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26867: New class: KConfigTypeInformation

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
tcanabrava requested review of this revision.

REVISION SUMMARY
  This class handles transformations and queries for xml types to
  cpp types.
  
  Use KConfigTypeInformation to get the cppType, type and defaultValue

TEST PLAN
  Run unittests, all pass.

REPOSITORY
  R237 KConfig

BRANCH
  rewrite_maps

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigTypeInformation.cpp
  src/kconfig_compiler/KConfigTypeInformation.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26202: Refactor KConfigXT

[Tomaz Canabrava]

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:95aee1294e32: Refactor KConfigXT (authored by tcanabrava).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=74118=74121

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

AFFECTED FILES
  autotests/kconfig_compiler/CMakeLists.txt
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.h.ref
  autotests/kconfig_compiler/test_emptyentries.kcfg
  autotests/kconfig_compiler/test_emptyentries.kcfgc
  autotests/kconfig_compiler/test_emptyentries_main.cpp
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigParameters.cpp
  src/kconfig_compiler/KConfigParameters.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/KConfigXmlParser.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: davidre, bcooksley, cgiboudeaux, kossebau, bport, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 74118.
tcanabrava marked an inline comment as done.
tcanabrava retitled this revision from "Refactor KConfigXT " to "Refactor 
KConfigXT".
tcanabrava added a comment.


  - Rebase

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73846=74118

BRANCH
  rework_kconfig_compiler

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

AFFECTED FILES
  autotests/kconfig_compiler/CMakeLists.txt
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.h.ref
  autotests/kconfig_compiler/test_emptyentries.kcfg
  autotests/kconfig_compiler/test_emptyentries.kcfgc
  autotests/kconfig_compiler/test_emptyentries_main.cpp
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigParameters.cpp
  src/kconfig_compiler/KConfigParameters.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXmlParser.cpp
  src/kconfig_compiler/KConfigXmlParser.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: davidre, bcooksley, cgiboudeaux, kossebau, bport, ngraham, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26768: Revert "Revert "WIP: Refactor KConfigXT""

[Tomaz Canabrava]

tcanabrava abandoned this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26202: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava marked 26 inline comments as done.
tcanabrava added inline comments.

INLINE COMMENTS

> ervin wrote in KConfigCodeGeneratorBase.cpp:80
> 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).

it's to stay close to the original. originally all the indentation was done 
manually, but first 4 spaces and later 2 spaces. This code is bogus and I would 
like to get rid of it spporting only one indentation style. I'm adding a TODO: 
for that here.

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

D26202: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  @ervin since they are just nitpicks I'll fix in code and land the patch.

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

D26202: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  In D26202#597044 , @davidre wrote:
  
  > I checked 
https://cgit.kde.org/kconfig.git/tree/src/kconfig_compiler/kcfg.xsd and it says
  >
  >   
  >  
  >  
  >  
  >   
  >
  > According to google default of `minOccurs` is 1 so empty files are indeed 
not allowed
  
  
  rigth now only kdevelop (wtha  patch open) and kmymoney are doing those (and 
in both cases they commented out / removed the entries a long time ago) - being 
'better safe than sorry' I'll leave as is till kf6

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

Re: D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

[Tomaz Canabrava]

I think we are misunderstanding eachother here.
If I'm developing the software and running cmake all the time, having a
warning that I can't fix (because it depends in another platform) is noise,
but still being a developer I don't want to run with -Wno-dev.
I do work in applications that targets more than one system (and they are
mac / windows / ios / android) I tend to use buildhosts, and those
buildhosts will tell me the warning - if any. But only if I'm targeting
them.

I don't see the gain on having a warning - in a windows system, about
missing mac icons if I'm not *deploying*.
nor I do see a warning on a linux system about windows or mac run time
issues (and missing icons is a run time issue).


On Sun, 19 Jan 2020 at 12:03 Tomaz Canabrava  wrote:

> That’s not a developer issue, it’s a packaging issue.
>
> On Sun, 19 Jan 2020 at 12:02 Christophe Giboudeaux <
> nore...@phabricator.kde.org> wrote:
>
>> cgiboudeaux added a comment. View Revision
>> <https://phabricator.kde.org/D26752>
>>
>> You may use Linux to develop software that's intended to be used also on
>> Mac and Windows. You can't expect developers to have build environment for
>> every platform
>>
>> *REPOSITORY*
>> R240 Extra CMake Modules
>>
>> *REVISION DETAIL*
>> https://phabricator.kde.org/D26752
>>
>> *To: *patrickelectric, apol, tcanabrava, cgiboudeaux
>> *Cc: *apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem,
>> LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
>>
>

D26202: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  @dfaure I tried to remove the `WIP` from the history but I'm worried that 
will force git push --force as I merged this with the wip before.
  any hints?

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: cgiboudeaux, kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D26202: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava retitled this revision from "WIP: Refactor KConfigXT" to "Refactor 
KConfigXT ".

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: cgiboudeaux, kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D26768: Revert "Revert "WIP: Refactor KConfigXT""

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
tcanabrava requested review of this revision.

REVISION SUMMARY
  This reverts commit 5f8c2ce63499d05dfb4753eb1acc21dccf21d434 
.
  
  Add Reference files for Broken KDevelop Configuration
  
  Fix generating of empty configuration files
  
  This seems a broken design and I'm keeping here just for compatibility
  with KF5 - but this should be removed on KF6. With this patch the
  generator will generate *empty* classes for configuration files without
  entries.
  
  By empty I mean that there will be no entries managed but everything else
  will still be there: constructor, d-pointer, static methods, etc.
  
  Not proud of this.

REPOSITORY
  R237 KConfig

BRANCH
  rework_kconfig_compiler

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

AFFECTED FILES
  autotests/kconfig_compiler/CMakeLists.txt
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.h.ref
  autotests/kconfig_compiler/test_emptyentries.kcfg
  autotests/kconfig_compiler/test_emptyentries.kcfgc
  autotests/kconfig_compiler/test_emptyentries_main.cpp
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26766: Revert "Revert "WIP: Refactor KConfigXT""

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
tcanabrava requested review of this revision.

REVISION SUMMARY
  Simplify If-Else chain inside of defaultValue function
  
  Use a type collection to verify which value we should return
  to the code generator. This code is slower than the original one
  but it's more maintenable.

REPOSITORY
  R237 KConfig

BRANCH
  simplify_defaultValue

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

AFFECTED FILES
  autotests/kconfig_compiler/CMakeLists.txt
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.h.ref
  autotests/kconfig_compiler/test_emptyentries.kcfg
  autotests/kconfig_compiler/test_emptyentries.kcfgc
  autotests/kconfig_compiler/test_emptyentries_main.cpp
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26766: Revert "Revert "WIP: Refactor KConfigXT""

[Tomaz Canabrava]

tcanabrava abandoned this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

[Tomaz Canabrava]

tcanabrava added a comment.


  That’s not a developer issue, it’s a packaging issue.

REPOSITORY
  R240 Extra CMake Modules

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

To: patrickelectric, apol, tcanabrava, cgiboudeaux
Cc: apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

Re: D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

[Tomaz Canabrava]

That’s not a developer issue, it’s a packaging issue.

On Sun, 19 Jan 2020 at 12:02 Christophe Giboudeaux <
nore...@phabricator.kde.org> wrote:

> cgiboudeaux added a comment. View Revision
> 
>
> You may use Linux to develop software that's intended to be used also on
> Mac and Windows. You can't expect developers to have build environment for
> every platform
>
> *REPOSITORY*
> R240 Extra CMake Modules
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26752
>
> *To: *patrickelectric, apol, tcanabrava, cgiboudeaux
> *Cc: *apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem,
> LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
>

D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

[Tomaz Canabrava]

tcanabrava added a subscriber: apol.
tcanabrava added a comment.


  But why would I get the warning if I build on Linux? The warning should
  target the platform, not the entire build system.

REPOSITORY
  R240 Extra CMake Modules

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

To: patrickelectric, apol, tcanabrava, cgiboudeaux
Cc: apol, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

Re: D26752: ECMAddAppIcon: Do not warn about mac and window icons if isnt a OS specific build

[Tomaz Canabrava]

But why would I get the warning if I build on Linux? The warning should
target the platform, not the entire build system.


On Sun, 19 Jan 2020 at 10:00 Christophe Giboudeaux <
nore...@phabricator.kde.org> wrote:

> cgiboudeaux requested changes to this revision.
> cgiboudeaux added a comment.
> This revision now requires changes to proceed. View Revision
> 
>
> I object. This warning is for developers. It tells them the icons are
> missing for some platforms.
>
> *REPOSITORY*
> R240 Extra CMake Modules
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26752
>
> *To: *patrickelectric, apol, tcanabrava, cgiboudeaux
> *Cc: *cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n,
> GB_2, bencreasy, michaelh, ngraham, bruns
>

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  Aparently the kmymoney issue was the same: empty kconfig file. I just 
successfully compiled kdevelop and kmymoney.
  I'll let the computer to compile the whole kde applications from scratch 
tonigth to see if it will fail somewhere.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: cgiboudeaux, kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73846.
tcanabrava added a comment.


  - Revert "Revert "WIP: Refactor KConfigXT""
  - Add Reference files for Broken KDevelop Configuration
  - Fix generating of empty configuration files

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73688=73846

BRANCH
  rework_kconfig_compiler

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

AFFECTED FILES
  autotests/kconfig_compiler/CMakeLists.txt
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.cpp.ref
  autotests/kconfig_compiler/test_emptyentries.h.ref
  autotests/kconfig_compiler/test_emptyentries.kcfg
  autotests/kconfig_compiler/test_emptyentries.kcfgc
  autotests/kconfig_compiler/test_emptyentries_main.cpp
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: cgiboudeaux, kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  who knew? This actually was not a false positive: the kdevelop build failure 
was a bug in kdevelop.
  I already opened a ticket: 
https://invent.kde.org/kde/kdevelop/merge_requests/90
  but at the same time I added code to handle the case of broken / empty 
kconfigxt.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: cgiboudeaux, kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D26752: ECMAddAppIcon: Do not warn about mac icons if isnt a mac build

[Tomaz Canabrava]

tcanabrava accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  warning_icons

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

To: patrickelectric, apol, tcanabrava
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D26751: ECMAddAppIcon: Add sc in regex to extract extension from valid names

[Tomaz Canabrava]

tcanabrava accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  sc_appicon

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

To: patrickelectric, tcanabrava
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava reopened this revision.
tcanabrava added a comment.
This revision is now accepted and ready to land.


  Reopening for review :)

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava requested review of this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  In D26202#595820 , @tcanabrava 
wrote:
  
  > We can revert, and I’ll fix the full build. Doing that now.
  
  
  I Just reverted this and I'm working on a full build of kde using 
kdesrc-build --refresh-build, I'll reopen this patch when *all* projects build 
sucessfully, with a unittest for each one that broke.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  We can revert, and I’ll fix the full build. Doing that now.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

Re: D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

We can revert, and I’ll fix the full build. Doing that now.

On Fri, 17 Jan 2020 at 09:03 Friedrich W. H. Kossebau <
nore...@phabricator.kde.org> wrote:

> kossebau added a comment. View Revision
> 
>
> where "full" would also need to mean "clean fresh build dirs" :) so that
> the codegenerator is triggered to be run, as the current cmake code seems
> to not inject a build dep between generator instance and generated files,
> so a new version does not trigger regeneration. Something that could also
> get a fix perhaps, IIRC other tools have such a dep.
>
> *REPOSITORY*
> R237 KConfig
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26202
>
> *To: *tcanabrava, Frameworks, ervin, bport, dfaure
> *Cc: *kossebau, bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2,
> michaelh, bruns
>

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

This revision was automatically updated to reflect the committed changes.
Closed by commit R237:98c32e29f504: WIP: Refactor KConfigXT (authored by 
tcanabrava).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73608=73688

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added inline comments.

INLINE COMMENTS

> dfaure wrote in kconfigcompiler_test.cpp:129
> OK, this needs a hint for the person debugging regressions then. Something 
> like
> 
>   QVERIFY2(content == contentRef, "Failure, see foo.diff");

This is done now within the appendFileDiff function.

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava marked an inline comment as done.

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73608.
tcanabrava added a comment.


  - Rebase

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73557=73608

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73557.
tcanabrava added a comment.


  - Add missing copyright holders

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73510=73557

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  Took the time to nuke the SignalArguments and use Param instead, easier than 
I initially tougth.

INLINE COMMENTS

> dfaure wrote in kconfigcompiler_test.cpp:180
> I meant  QVERIFY2(diffFile.open(...), ...).
> No need to make a separate call to isOpen().

ups :)

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73510.
tcanabrava marked 10 inline comments as done.
tcanabrava added a comment.


  - Simplify List creation
  - Fail with the diff file name
  - Simplify Code
  - Fix Typo
  - Remove `SignalArgument` for `Param`
  - Nitpicks and Include fixes

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73345=73510

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  @dfaure , @ervin
  
  Right now the code passes all the tests and I tried to be extra careful not 
to break away with the general architecture. I think it's the first "safe" 
version to review.

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73345.
tcanabrava added a comment.


  - Fix bug ediging param variable that's supposed to be const
  - Const Correctness:

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73336=73345

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava marked 8 inline comments as done.

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added inline comments.

INLINE COMMENTS

> tcanabrava wrote in kconfig_compiler.cpp:753
> that was a bit harder than I want, but done. Inside of the code generation 
> there was code that manipulated the ParseResult. I think this is one of the 
> good spots that show that this rewrite is really needed.

aand no, this introduced regressions, I'll try to solve it later but it's 
not as simple. this will unfortunately still be modified inside of the 
generator files. :/

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73336.
tcanabrava marked 4 inline comments as done.
tcanabrava added a comment.


  - Rename KConfigCodeGenerator to KconfigCodeGeneratorBase
  - Simplify diff file saving
  - Add License Headers
  - Documentation, Privatization, Unused method / variable
  - Const, Documentation
  - Newlines at the end of files
  - Spacing
  - Remove Globals
  - Remove unused code and Debugs
  - Constify ParseResult
  - English Typos
  - qAsConst
  - Public / Private issues
  - Simplify Construction
  - Revert "Constify ParseResult"

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73310=73336

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava marked 22 inline comments as done.
tcanabrava added inline comments.

INLINE COMMENTS

> dfaure wrote in kconfigcompiler_test.cpp:129
> 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...

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.

> dfaure wrote in KConfigCodeGenerator.h:91
> urgh, a public variable

that was a honest mistake :)

> dfaure wrote in KConfigCommonStructs.h:14
> done already?

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.

> dfaure wrote in KConfigHeaderGenerator.h:73
> I thought most editors took care of that, these days...

Kate / KDevelop here. I'm manually adding them.

> dfaure wrote in KConfigSourceGenerator.cpp:40
> remove (or switch to qCDebug)

removed, those are temporaries debug calls that I used to be sure things are as 
supposed.

> dfaure wrote in kconfig_compiler.cpp:753
> const ... ?

that was a bit harder than I want, but done. Inside of the code generation 
there was code that manipulated the ParseResult. I think this is one of the 
good spots that show that this rewrite is really needed.

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  Looking at the review you it's a bis strange to see that I'v touched some 
test reference generated code, I did this only on *whitespace only changes* 
where the new whitespace made more sense than the old one.
  there is *one* bug I havent fixed on this code that I plan to fix tomorrow, 
as it's almost 9:45 and it's saturday. I believe that this code is getting near 
review quality.
  I'v also rebased against master as some other people also started to change 
this code - I already got a few conflicts, so I really need to rush this before 
it gets unmergeable.

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73310.
tcanabrava added a comment.


  - Fix whitespace on the reference code / genreator

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73301=73310

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73301.
tcanabrava added a comment.


  - More whiteespaces fixed
  - Fix *space only* issues with the  test reference headers
  - More whitespace fixes, only six files to go
  - Way more tests passing - space fixes

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73292=73301

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73292.
tcanabrava added a comment.


  - Fix many small whitespace issues

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73290=73292

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73290.
tcanabrava added a comment.


  - Rebase on master

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72715=73290

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26573: Add missing Import Env Variable

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added projects: Frameworks, Build System.
Herald added subscribers: kde-buildsystem, kde-frameworks-devel.
tcanabrava requested review of this revision.

REVISION SUMMARY
  Without this, in Qt 5.14 I get an android-like QQC2 theme
  This used to work on Qt 5.13 so I assume that it's a regression

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  add_missing_env_var

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

AFFECTED FILES
  kde-modules/prefix.sh.cmake

To: tcanabrava
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72715.
tcanabrava edited the test plan for this revision.
tcanabrava added a comment.


  - Add Hack for Enums
  - Fix Whitespace diff
  - Move some raw scopes to start/end scope()

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72711=72715

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava edited the summary of this revision.
tcanabrava edited the test plan for this revision.

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72711.
tcanabrava added a comment.


  - Add missing .h
  - Fixes placement of code
  - Change how tests save tests
  - Change how the Indentation is done
  - Fix filename in the preamble
  - Separate logic to make function readable
  - Fix more newlines
  - Add Hacks around code to generate the same amount of Empty Space

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72377=72711

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a subscriber: bport.
tcanabrava added a comment.


  Not really as all the calls are the same, just split into classes and
  logical bits. I haven’t write a single line of logic.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

Re: D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

Not really as all the calls are the same, just split into classes and
logical bits. I haven’t write a single line of logic.

On Mon, 30 Dec 2019 at 19:00 Nathaniel Graham 
wrote:

> ngraham added a comment. View Revision
> 
>
> At this point, it seems more like a total rewrite than a refactor...
>
> *REPOSITORY*
> R237 KConfig
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26202
>
> *To: *tcanabrava, Frameworks, ervin, bport
> *Cc: *ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
>

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72377.
tcanabrava added a comment.


  - Reorganize code
  - Fix filename
  - Fix Static Methods
  - Fix name generation
  - Call forgotten function in the right place
  - Fix signal generation - remove stray bit of code

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72371=72377

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72371.
tcanabrava added a comment.


  - Regression: Remove space from the autogen preamble
  - Whitespace regression
  - Fix hasNamespace: logic was inverted
  - Space issues and scope
  - Use whitespace() function to manage indentation level
  - Space fixes and minor nitpicks

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72349=72371

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72349.
tcanabrava added a comment.


  - Fix some obvious bugs.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72338=72349

BRANCH
  megaRefactor

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava edited the summary of this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava edited the summary of this revision.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72338.
tcanabrava added a comment.


  - Move XmlParser to another file.
  - Move Code around again
  - Fix some issues - still broken
  - Move Parser to own file
  - Move a lot of code to the Parser Class
  - Fix a few of the many many warnings.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72326=72338

BRANCH
  megaRefactor

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72326.
tcanabrava added a comment.


  - Add new Class, KConfigSourceGenerator, fix usage of addHeader
  - Move CreateSingletonImplementation
  - Create Cpp Preamble
  - Move Constructor implementation
  - move Getter / Setter / Destructor
  - Remove unused function
  - Move nonModifyingSignalsHelper
  - Finished Moving generator code

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72171=72326

BRANCH
  megaRefactor

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72171.
tcanabrava added a comment.


  - Finish header / Move basename to the ParameterConfiguration

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72166=72171

BRANCH
  megaRefactor

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 72166.
tcanabrava added a comment.


  - Port more header functions
  - Move CreateItemAcessors and CreateSignals
  - Remove indentation level
  - Move NonDPointerMembers and DPointerMembers

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=72126=72166

BRANCH
  megaRefactor

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

Re: D26126: Simplify param method: Return Early, Use a Map and Assert.

[Tomaz Canabrava]

David,

Thanks for the detailed explanation, I'm currently reworking most of the
patches here. About the optimizations - I know the linear search is a bad
optimization, but it adds legibility, that's what I tried to do.
I tried to emulate the python `if value in vector` that is really easy to
read, compared to the current code.



On Tue, Dec 24, 2019 at 9:40 AM David Faure 
wrote:

> dfaure requested changes to this revision.
> dfaure added inline comments. View Revision
> 
> *INLINE COMMENTS*
> View Inline 
> kconfig_compiler.cpp:1020
> } else if (type == QLatin1String("font")) {
> return QStringLiteral("const QFont &");
> } else if (type == QLatin1String("rect")) {
> QLatin1String("double")}
> ).contains(type)) {
> return type;
>
> This linear search doesn't look like an optimization to me. It would be
> better to incorporate this into the map, so that a single search is
> performed, rather than two.
>
> View Inline ervin wrote
> in kconfig_compiler.cpp:1024
>
> std::map looks like a bad idea here, either use QHash (preferred in
> massively based Qt code) or std::unordered_map.
>
> Also for both temporaries you pay the price of their allocation and
> deallocation at each call. Even a mutex used at each call would do better.
> ;-)
>
> I'm not 100% sure about std::map vs QHash given the number of elements,
> this would need benchmarking.
>
> But I agree 100% that compiler-generated thread safety around a static is
> NOTHING compared to amount of nodes allocated to fill in a map or hash.
>
> @tcanabrava  Please have a
> look at https://gist.github.com/jboner/2841832
> Locking an available mutex is 4 times faster than even fetching data from
> main memory (i.e. data which isn't yet in a CPU cache). This is many orders
> of magnitude faster than creating a filling a map or a hash full of
> QStrings. On top of that, compilers don't even generate a full-blown mutex
> for threadsafe statics, they generate a three-state atomic (a bit like
> Q_GLOBAL_STATIC does) (3 because not created, being created, already
> created).
>
> The most performant solution is to turn the input string into a QByteArray
> and then perform a binary search in a C array of const char* (no
> allocations, even the very first time).
> The most readable (but still good, unlike the current code in git)
> solution is a static map.
>
> *REPOSITORY*
> R237 KConfig
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26126
>
> *To: *tcanabrava, ervin, dfaure
> *Cc: *dfaure, ervin, GB_2, apol, kde-frameworks-devel, LeGast00n,
> michaelh, ngraham, bruns
>

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  In D26202#582541 , @ngraham wrote:
  
  > > This is really a wip, completely broken. It's just so people can point me 
to the right direction.
  >
  > Maybe could you clarify which direction you want to be pointed in? What's 
the goal here? What do you need help with?
  
  
  Fair Enough:
  
  The current KConfigXT compiler is in a sad state: 
  It's a massive file with loads of global variables that handle state, the 
generator is done within the main() function and it seems to have grown 
organically. There are no classes to separate logic / state / generation, what 
exists is code that generates code from a xml / ini pair, but it's hard to even 
discover what a bit of code is doing. The code istyle is C++ / Java from the 
nineties, which is not bad per see but it also uses quite a few things that are 
going to be deprecated in Qt 6 so I'm also taking the time make the code more 
streamlined with newer code style (no iterators, lambdas, auto usage, etc).
  
  The code that generates the files simplly pushes strings to a text stream, 
and it's hard to figure out when something starts or something ends: for 
instance, the code that generates the Constructor has more than sixty lines of 
code englobing some nested if - for - if - for constructs.
  
  This code tries to Separate the compiler code into many different files / 
classes to be more obvious what's happening, and each class also has many 
helper methods to minimize copypaste.
  
  - CodeGenerator: Has base code for the header and source files that can be 
shared
  - HeaderGenerator: Logic for generating the header file
  - SourceGenerator: Logic for generating the source file
  - KcfgParser: Logic for parsing the kcfg file and extracting the information
  - CommonStructs: a header that contains the structs that are currently used 
everywhere.
  - KConfigParameters: (was CfgConfig - ConfigConfig, wat) - Has information 
passed via the kcfgc file
  - kcfg_compiler - will be renamed to main - start the other classes and 
generates the files.
  
  This code here currently has the begining of this separation, with the 
CodeGenerator and the HeaderGenerator in a ~good~ state, but unfinished.

REPOSITORY
  R237 KConfig

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

To: tcanabrava
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

Re: D26127: Simplify cppType method: Return Early, Use a Map and Assert.

[Tomaz Canabrava]

doing that as we speak.

On Mon, Dec 23, 2019 at 9:52 PM Kevin Ottens 
wrote:

> ervin added a comment. View Revision 
>
> In D26127#582398 , @tcanabrava
>  wrote:
>
> In D26127#582296 , @ervin
>  wrote:
>
> Those data structure look really similar to the ones you introduced in
> param() for D26126 . It looks like
> we'll end up with a Q_GLOBAL_STATIC or such instead of code duplication.
>
> it's similar but the values are different, the other one is for
> parameters, this one for return types, so on the other one we have const
> QList & and here is just QList.
> I tougth in a way to make them both be the same, but I couldn't find a
> way, since some elements will not have the const & in both cases, like
> int, uint, double, I prefered to keep the maps separated.
>
> Well sure, it can't be shared "as is", clearly there's a richer type
> missing to tie it all together. This could actually be the start of a
> domain model in that application.
>
> *REPOSITORY*
> R237 KConfig
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D26127
>
> *To: *tcanabrava, ervin
> *Cc: *ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham,
> bruns
>

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
tcanabrava requested review of this revision.

REVISION SUMMARY
  This is really a wip, completely broken. It's just so people can point me to
  the right direction.
  
  Rework CfgEntry
  
  Remove commented code
  
  Add base class `KConfigCodeGenerator`, a helper to generate the Header and 
Source files
  
  Add new KConfigHeaderGenerator / Share the common Structs
  
  KConfigHeaderGenerator will generate the Header,
  it uses the same structs as the kconfig_compiler.cpp
  
  Create a Struct for the parse results and use it.
  
  Fix header Generation
  
  Implement the Enums / Move Quite of code around
  
  Moved Create Constructor
  
  Move Destructor
  
  Move `This` and `Const` special Vars to ConfigCodeGenerator

REPOSITORY
  R237 KConfig

BRANCH
  megaRefactor

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

AFFECTED FILES
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KConfigCodeGenerator.cpp
  src/kconfig_compiler/KConfigCodeGenerator.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26128: Simplify defaultValue method: Return Early, Use Default Initialization, and Assert.

[Tomaz Canabrava]

tcanabrava added inline comments.

INLINE COMMENTS

> apol wrote in kconfig_compiler.cpp:1122
> If instead of creating a QVector you used an initializer list directly, you 
> could use std::find_if without having to initialize it entirely. Also you'd 
> be able to use QLatin1String which would save constructing unneeded QStrings.

.contains adds way more readability than 
std::find_if(std::begin(temporaryStringVector), std::end(temporaryStringVector) 
!= std::end(temporaryStringVector), so I opted for this.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26127: Simplify cppType method: Return Early, Use a Map and Assert.

[Tomaz Canabrava]

tcanabrava added a comment.


  In D26127#582296 , @ervin wrote:
  
  > Those data structure look really similar to the ones you introduced in 
param() for D26126 . It looks like we'll 
end up with a Q_GLOBAL_STATIC or such instead of code duplication.
  
  
  it's similar but the values are different, the other one is for parameters, 
this one for return types, so on the other one we have `const QList &` 
and here is just `QList`.
  I tougth in a way to make them both be the same, but I couldn't find a way, 
since some elements will not have the `const &` in both cases, like int, uint, 
double, I prefered to keep the maps separated.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26128: Simplify defaultValue method: Return Early, Use Default Initialization, and Assert.

[Tomaz Canabrava]

tcanabrava added a comment.


  In D26128#582304 , @ervin wrote:
  
  > This also has similarities with D26126 
, has the same defects and missed 
opportunities for sharing.
  >
  > Beside I'm not sure what we're trying to achieve here, those parts of 
kconfig_compiler are not that bad. I'm not a huge fan of series of "else if" 
but that's not really making kconfig_compiler hard to understand, the overall 
lack of consistent domain model and the way the code is generated on the other 
hand...
  
  
  This is trying to make the code easier to read. The if chain has more than 60 
lines of one liners, this reduces to half and improves readability and 
maintenance.
  It's true that this part of the kconfig_compiler are not that bad, but it's 
an opportunity to make them better.
  
  I'm also touching the other parts: the model, the domain, the code generation 
etc.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26129: Remove Iterator based loops for range based loops

[Tomaz Canabrava]

tcanabrava added inline comments.

INLINE COMMENTS

> ervin wrote in kconfig_compiler.cpp:2119
> This logic is very flawed... instead of comparing positions in the list now 
> you're comparing actual values. If two different positions in the list would 
> end up having the same values (for some reason) we'd generate syntactically 
> incorrect code.

if two items in the list have the same values the KConfig XML is wrong and the 
generated code will not compile. I don't see an issue with it.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26129: Remove Iterator based loops for range based loops

[Tomaz Canabrava]

tcanabrava added a comment.


  In D26129#582336 , @ervin wrote:
  
  > The patch being large I didn't check each one separately but I suspect this 
patch misses boat loads of qAsConst.
  
  
  Yes, it indeed misses, I'll add the qAsConst. ( I would actually prefer to 
change the QList to std::vector in the future, no need to add the 'qAsConst' 
there and it would help readability )
  
  > Besides there's at least one case of very flawed logic, it can't be about 
blindly switching one loop construct for another especially when the iterators 
are compared inside the loop body.
  
  I didn't blindly change anything, everything I did I tested either by running 
the unittests or recompilling and testing the settings of applications.
  
  This patch greatly increases the readability of the software. I'll do another 
round of reviews here adding the qAsConst and trying to find broken logic but I 
don't believe they are really broken.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, ervin
Cc: ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns