improve documentation for DsymbolTable #12570
Conversation
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#12570" |
| Dsymbol* ps = tab.getLvalue(ident); | ||
| *ps = s; | ||
| return s; | ||
| *tab.getLvalue(s.ident) = s; |
There was a problem hiding this comment.
Revert this, it's not an improvement to the documentation.
(Also forgot to update dsymbol.h...)
There was a problem hiding this comment.
This PR is tagged "refactoring".
Updated dsymbol.h.
There was a problem hiding this comment.
A refactoring should not change the behavior... Have you checked that this change doesn't affect LDC?
There was a problem hiding this comment.
It doesn't change the behavior - dmd never calls it. As for gdc/ldc, if they're using the return value, it'll give an error, not silently corrupt anything. The return value makes no sense.
There was a problem hiding this comment.
It occurs to me you and I have different interpretations of what "refactoring" means. To me, it means not changing the behavior of the program. To you (correct me if I'm wrong) it means not changing the API of any functions.
There was a problem hiding this comment.
Refactorings should retain the behavior of a component, it's more nuanced than the whole program but doesn't prohibit changes to function signatures.
The problem here is a change to the public API which can break code depending on "DMD as a library" - without providing any real benefit.
But I guess that is acceptable given that the potential breakage isn't silent and can easily be fixed.
Leave it better than I found it.