Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5081)

Issue 559250043: Fix hidden member templates in derived classes

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 2 days ago by hahnjo
Modified:
2 weeks, 5 days ago
Reviewers:
dak, lemzwerg, Dan Eble
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Fix hidden member templates in derived classes According to Clang's interpretation of the C++ standard, "member functions in the derived class override and/or hide member functions with the same name and parameter types in a base class". This seems to also hold for templated member functions and even if the code has a using-declaration of the base class member. This affects LilyPond's implementation of the method_finder template. Luckily, the problem can be solved by just re-declaring the base template specializations in each derived class instead of 'using' them. The only drawback is that this doesn't work out-of-the-box for deeper inheritance hierarchies. However, this only affects three ligature engravers so I think it's worth the maintenance to just encode the transitive inheritance manually.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Move comment that I overlooked #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -9 lines) Patch
M lily/include/translator.hh View 1 2 chunks +9 lines, -9 lines 0 comments Download
M lily/kievan-ligature-engraver.cc View 1 chunk +1 line, -0 lines 0 comments Download
M lily/mensural-ligature-engraver.cc View 1 chunk +1 line, -0 lines 0 comments Download
M lily/vaticana-ligature-engraver.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 9
lemzwerg
Wait, this tiny patch makes lilypond compilable with clang, passing the whole test suite?
3 weeks, 2 days ago (2019-11-14 08:45:44 UTC) #1
hahnjo
On 2019/11/14 08:45:44, lemzwerg wrote: > Wait, this tiny patch makes lilypond compilable with clang, ...
3 weeks, 2 days ago (2019-11-14 08:49:43 UTC) #2
lemzwerg
> And yes, this works for me, at least on Linux (didn't test FreeBSD yet ...
3 weeks, 2 days ago (2019-11-14 09:11:35 UTC) #3
dak
On 2019/11/14 09:11:35, lemzwerg wrote: > > And yes, this works for me, at least ...
3 weeks, 2 days ago (2019-11-14 11:29:25 UTC) #4
lemzwerg
Great! Jonas, please add comments to the source code that points to the relevant gcc ...
3 weeks, 2 days ago (2019-11-14 11:59:34 UTC) #5
Dan Eble
https://codereview.appspot.com/559250043/diff/547250043/lily/include/translator.hh File lily/include/translator.hh (right): https://codereview.appspot.com/559250043/diff/547250043/lily/include/translator.hh#newcode204 lily/include/translator.hh:204: // Fallback for non-overriden callbacks for which &T::x degrades ...
3 weeks, 2 days ago (2019-11-14 13:32:47 UTC) #6
hahnjo
On 2019/11/14 11:29:25, dak wrote: > [...] > I am not a fan of having ...
3 weeks, 2 days ago (2019-11-14 16:26:42 UTC) #7
lemzwerg
> > Great! Jonas, please add comments to the source code > >that points to ...
3 weeks, 2 days ago (2019-11-14 17:28:23 UTC) #8
hahnjo
2 weeks, 5 days ago (2019-11-18 17:42:35 UTC) #9
Move comment that I overlooked
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b