[Kde-bindings] KSharedConfig multiple matches

[Richard Dale]

On Friday 29 January 2010 09:18:31 pm Stefano Crocco wrote:
> On Friday 29 January 2010, David Palacio wrote:
> > |$ irb
> > |irb(main):001:0> kdeapp
> > |=> #<KDE::Application:0x7ff8a870b158 objectName="irb">
> > |irb(main):002:0> Qt.debug_level = 2
> > |=> 2
> > |irb(main):003:0> KDE::Global.config.group 'Recent'
> > |classname    == KGlobal
> > |
> > |:: method == config
> > |
> > |-> methodIds == [#<Qt::Internal::ModuleIndex:0x7ff8962612b8 @smoke=8,
> > |@index=1343>]
> > |candidate list:
> > |        static KSharedPtr<KSharedConfig> KGlobal::config()  (smoke: 8
> > |index: 1343)
> > |matching => smoke: 8 index: 1343
> > |match => 1343 score: 0
> > |Resolved to id: 1343
> > |setCurrentMethod(smokeList index: 8, meth index: 1343)
> > |classname    == KSharedConfig
> > |
> > |:: method == group
> > |
> > |-> methodIds == [#<Qt::Internal::ModuleIndex:0x7ff896260a70 @smoke=8,
> > |@index=489>, #<Qt::Internal::ModuleIndex:0x7ff896260a48 @smoke=8,
> > |@index=490>, #<Qt::Internal::ModuleIndex:0x7ff896260a20 @smoke=8,
> > |@index=492>, #<Qt::Internal::ModuleIndex:0x7ff8962609f8 @smoke=8,
> > |@index=493>] candidate list:
> > |        KConfigGroup KConfigBase::group(const QString&)
> > |(smoke: 8 index: 489)
> > |        KConfigGroup KConfigBase::group(const char*)
> > |(smoke: 8 index: 490)
> > |        const KConfigGroup KConfigBase::group(const QString&) const
> > |(smoke: 8 index: 492)
> > |        const KConfigGroup KConfigBase::group(const char*) const
> > |(smoke: 8 index: 493)
> > |matching => smoke: 8 index: 489
> > |      const QString& (s)
> > |match => 489 score: 4
> > |matching => smoke: 8 index: 490
> > |      const char* (s)
> > |match => 490 score: 1
> > |matching => smoke: 8 index: 492
> > |      const QString& (s)
> > |multiple methods matching, this is an error
> > |match => 492 score: 4
> > |matching => smoke: 8 index: 493
> > |      const char* (s)
> > |match => 493 score: 1
> > |TypeError: can't convert false into Integer
> > |        from /home/kde/ruby/lib/Qt/qtruby4.rb:2725:in `findAllMethods'
> > |        from /home/kde/ruby/lib/Qt/qtruby4.rb:2725:in
> > | `do_method_missing' from (irb):3:in `method_missing'
> > |        from (irb):3
> > |        from :0
> 
> I reported this issue a couple of months ago, but it hasn't been fixed as
>  yet, even if its cause has been understood. It seems that the new smoke
>  generator differentiates between const and non-const version of C++
>  methods, while the old generator didn't. This means that now it's not
>  enough to examine the argument list to find out which C++ method to call,
>  which is what qtruby does.
> 
> In the last days, I tried to fix the issue myself and (at least at a first
> galance) I succeeded. The attached patch changes the qtruby.cpp and
>  qtruby.rb files to allow to distinguish among overloaded methods which
>  only differ because of the const-ness either of the methods themselves or
>  of their arguments.
> 
> This is a summary of the changes I made:
> 
> - qtruby.cpp
> 
> Defined a isConstMethod method for the Qt::Internal module which returns
>  true if the method corresponding to the index passed as argument has the
>  const attribute and false otherwise
> 
> - qtruby.rb
> 
> Changed the way do_method_missing behaves if it finds two methods with the
> same score: now, instead of giving an error immediately, it checks whether
>  one is const and the other isn't (using the isConstMethod from
>  qtruby.cpp). If this is the case, the non-const method is chosen (which,
>  if I understand correctly, is what happened with the old smoke). If both
>  are const, or not const, the behaviour remains the same.
> 
> Initially, I thought the above changes should have been enough. However, it
> wasn't so. In particular, while KDE::SharedConfig#group worked,
> KDE::ConfigGroup.new didn't. I think the reason is that there's also an
> ambiguity with const/non const arguments. To avoid it, I also changed the
> Internal.checkarg method, so that a const argument score one less than a
>  non const argument of the same type would. Of course, to avoid clashes
>  between score for a const argument of one type and a non-const argument of
>  another type, I had to change the score of all of them, so that now they
>  differ by two, rather than by one as it used to be.
> 
> I don't know whether this changes work correctly. I tried them with my
> application and they seem to work without drawbacks, but I must admit I had
> not much time to test them. However, I hope they can be useful.
Thanks for the patch - i'm sorry I've taken so long to have a look at this. In 
fact I was about to fix it, when I discovered there was already a 
'isConstMethod()' that I was about to add. It seems Arno had already applied 
your patch.

I was going to give a const method a score of 1, rather than have this special 
case test at the end of the matching:

if !isConstMethod(id) and isConstMethod(chosen)
    chosen = id
elsif isConstMethod(id) == isConstMethod(chosen)
puts "multiple methods matching, this is an error" if debug_level 
>=DebugLevel::Minimal
   chosen = nil
end

But if it works I think we should leave it as per your patch.

The overloading on const/non-const arguments issue is something I don't even 
know was a problem, but I think your solution of giving a score of 1 for const 
args is correct.

-- Richard