Skip to content

Allow configureable WhitespaceBehavior in getTokensForParser#149

Closed
wilzbach wants to merge 2 commits intodlang-community:masterfrom
wilzbach:fix-whitespace-include
Closed

Allow configureable WhitespaceBehavior in getTokensForParser#149
wilzbach wants to merge 2 commits intodlang-community:masterfrom
wilzbach:fix-whitespace-include

Conversation

@wilzbach
Copy link
Member

Needed for dlang-community/D-Scanner#448 and and dlang-community/D-Scanner#450

I am not sure whether this is the best solution as it requires allocation by the user as const(Token)[] is

	tokens = getTokensForParser(code, config, &cache);
	auto tokensC = tokens; // <-- this can be used by the Dscanner checks
	if (config.whitespaceBehavior == WhitespaceBehavior.include)
		tokensC = tokens.filter!(t => t.type != WhitespaceBehavior.include).array;
...
	dparse.parser.parseModule(tokensC, fileName, p, report, ...)
...

The parser currently doesn't check for whitespace tokens and imho it would unnecessarily complicate its logic to add checks.

@wilzbach
Copy link
Member Author

(rebased to see how CodeCov turns out to work)

@codecov-io
Copy link

codecov-io commented Jun 21, 2017

Codecov Report

Merging #149 into master will increase coverage by 4.32%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   81.47%   85.79%   +4.32%     
==========================================
  Files           7        7              
  Lines        4528     7041    +2513     
==========================================
+ Hits         3689     6041    +2352     
- Misses        839     1000     +161
Impacted Files Coverage Δ
src/dparse/lexer.d 84.31% <40%> (+4.29%) ⬆️
src/dparse/ast.d 14.03% <0%> (-1.11%) ⬇️
test/tester.d 100% <0%> (ø) ⬆️
src/dparse/rollback_allocator.d 89.79% <0%> (+0.21%) ⬆️
src/std/experimental/lexer.d 86.66% <0%> (+2.94%) ⬆️
src/dparse/parser.d 90.48% <0%> (+3.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b009b7...90c37c1. Read the comment docs.

@wilzbach wilzbach force-pushed the fix-whitespace-include branch from fb5c14c to d9c9a83 Compare June 28, 2017 05:16
Copy link
Collaborator

@Hackerpilot Hackerpilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getTokensForParser function was given that name deliberately. The parser doesn't handle whitespace tokens so there should not be any whitespace in the value returned from this function.

{
case tok!"specialTokenSequence":
case tok!"whitespace":
if (config.whitespaceBehavior == WhitespaceBehavior.include)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (unchanged) doc comment specifically states that this won't happen.

@wilzbach
Copy link
Member Author

The getTokensForParser function was given that name deliberately. The parser doesn't handle whitespace tokens so there should not be any whitespace in the value returned from this function.

Ok, but

  1. What's the purpose of config.whitespaceBehavior then?
  2. How do you propose to allow getting the whitespace tokens from libdparse then?(as e.g. needed for r [WIP] Fix #328 - Add check for trailing whitespace D-Scanner#448 or Add check for if constraint indendation D-Scanner#450)

@Hackerpilot
Copy link
Collaborator

  1. That config option is used inside of the lexer's popFront function.
  2. It may be a good idea to attach the whitespace tokens to the non-whitespace tokens the way we do with comments.

WebFreak001 added a commit to WebFreak001/libdparse that referenced this pull request Apr 8, 2020
This replaces the previous comment and trailingComment properties.
Maintains full feature backwards-compatibility, only some `@nogc` code
may now fail to compile if it used the comment or trailingComment
properties from Token.

supersedes dlang-community#149
WebFreak001 added a commit to WebFreak001/libdparse that referenced this pull request Apr 8, 2020
This replaces the previous comment and trailingComment properties.
Maintains full feature backwards-compatibility, only some `@nogc` code
may now fail to compile if it used the comment or trailingComment
properties from Token.

supersedes dlang-community#149
WebFreak001 added a commit to WebFreak001/libdparse that referenced this pull request Apr 8, 2020
This replaces the previous comment and trailingComment properties.
Maintains full feature backwards-compatibility, only some `@nogc` code
may now fail to compile if it used the comment or trailingComment
properties from Token.

supersedes dlang-community#149
@WebFreak001
Copy link
Member

there is a new PR (#387) which would replace this PR

@WebFreak001
Copy link
Member

WebFreak001 commented Apr 17, 2020

closing because no longer needed (superseded by #387, merged)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants