Fix duplicate-skipping in prototype generation #154
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While generating prototypes, a scan is made for any function definitions which would result in identical prototypes, and all but the first are skipped. However, when doing so, this did not take into account function definitions which were already marked to be skipped, which could cause *all* definitions to be skipped. This behaviour caused a particular problem when a method definition existed with the same signature as a function definition. All method definitions are skipped through `tagIsUnhandled`, but could still make `skipDuplicates` skip the corresponding function definition. This commit fixes this. It is not entirely clear to me if this duplicate removal is actually needed at all. If code contains multiple function definitions with the same signature, you would expect the compiler to throw an error anyway. If duplicate removal would be removed, only one testcase (TestCTagsToPrototypesShouldDealFunctionWithDifferentSignatures) fails, but that testcase uses a predefined ctags output that contains a duplicate function definition. It is not quite clear what source files this output was generated from (it seems to stem from CharWithEscapedDoubleQuote.ino, which originated at arduino/Arduino#1245, but those only contain one version of the function definition, not two). Still, since the ctags parsing will hopefully be replaced in the near future, this just fixes the duplicate removal instead of removing it and risking a regression on some corner case. This fixes arduino#140. Signed-off-by: Martino Facchin <[email protected]> Signed-off-by: Matthijs Kooijman <[email protected]>
This was referenced May 18, 2016
|
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
|
Rebased and merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While generating prototypes, a scan is made for any function definitions
which would result in identical prototypes, and all but the first are
skipped. However, when doing so, this did not take into account function
definitions which were already marked to be skipped, which could cause
all definitions to be skipped.
This behaviour caused a particular problem when a method definition
existed with the same signature as a function definition. All method
definitions are skipped through
tagIsUnhandled, but could stillmake
skipDuplicatesskip the corresponding function definition. Thiscommit fixes this.
It is not entirely clear to me if this duplicate removal is actually
needed at all. If code contains multiple function definitions with the
same signature, you would expect the compiler to throw an error anyway.
If duplicate removal would be removed, only one testcase
(TestCTagsToPrototypesShouldDealFunctionWithDifferentSignatures) fails,
but that testcase uses a predefined ctags output that contains a
duplicate function definition. It is not quite clear what source files
this output was generated from (it seems to stem from
CharWithEscapedDoubleQuote.ino, which originated at
arduino/Arduino#1245, but those only contain
one version of the function definition, not two).
Still, since the ctags parsing will hopefully be replaced in the near
future, this just fixes the duplicate removal instead of removing it and
risking a regression on some corner case.
This fixes #140.
Signed-off-by: Martino Facchin [email protected]
Signed-off-by: Matthijs Kooijman [email protected]