nimble-dev
allow non-abstract do-nothing default methods in nimbleFunctionVirtual
This is not for the imminent release.
@danielturek This PR provides the features needed to modify #1181 so that sampler_BASE could be expanded rather than adding a sampler_BASE2. Let's see if it passes tests on devel and if so pull it into ADoak. An example of how to use it is in the new final test in test-checkDSL.R. I put it there because that's where nimbleFunctionVirtual appears in a test, but actually the new test could fit better in a different test file, but I'm leaving it to run tests for now.
@danielturek There was a CI glitch in the first round of testing. I re-ran tests and they passed. So I think this is good to go for you to work from.
@perrydv Can we merge this PR (1) into devel, and (2) then into ADoak ? That would be my preference, as soon as we're ready, so I can move forward with other changes and testing.
After this is merged into devel, please note if you/we are not prepared for the "full" merge of devel branch into ADoak (which I would understand), then it's easy to just merge this one single commit existing on devel branch into ADoak. IIRC, the git commands would be:
git checkout ADoak
git cherry-pick [COMMIT HASH TO MERGE IN]
That will merge only that one commit on devel branch into ADoak.
@perrydv Also, what would you think about changing the name of abstract (in the new methodControl list) to something more intuitive? Something that conveys whether a default (do-nothing) implementation will be provided, or not? Or, conversely, whether an implementation will be required. Like required, or default_provided? This seems more intuitive to users and programmers who aren't intimately familiar with C++.
@perrydv Along the same lines, it also feels like a "double-negative" having to provide abstract = FALSE, in order to specify that I do want a default to be provided.
@perrydv Also (sorry for all the comments), why don't we just make all methods non-abstract, for all nimbleFunctionVirtual methods? (and skip the distinction between two different types of abstract / non-abstract methods, entirely). What would be the harm, really?
@danielturek Thanks for the comments. Naming: Let's go with required. Default: If we change the default to FALSE (current behavior corresponds to TRUE), we are changing the API slightly. However, as maybe you are saying, it shouldn't be possible that this will break any existing code, because all methods have been required anyway. @paciorek, any thoughts here?
Something to note: With required = FALSE, for a non-void return type, the default function will compile, and will return some kind of object of the declared type, but the behavior is officially undefined in C++ and could trigger compiler warnings on user's systems depending on their settings.
@perrydv Naming the argument required sounds good.
My personal feeling is to make the default be required = FALSE, but I do understand about changing the API. I'll defer to your judgement there, and any thoughts from @paciorek.
Maybe it's worth doing some testing about compiler warnings, related to the non-void return type? Ideally on some different platforms? I'm not sure...
I changed the name to required. I think in addition to compiler warning, it is possible to have crashes is a default method with a return type but not return function is actually called. It would be possible to flesh out the class inheritance system to have actual base class method definitions, but that is not otherwise a high priority goal. So potentially the safer course is to leave the default as is (required = TRUE).
@danielturek I merged this branch into ADoak, so you should be able to work there as you want.
@perrydv Since this PR seems to be merged into ADoak and AD-rc0 branches, can we safely close this PR?
@paciorek @danielturek Yes I don't see a problem with closing this PR here and leaving the changes to AD branches, where they are.