Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rules] Improving removal of comments #4774

Merged

Conversation

tonhuisman
Copy link
Contributor

@tonhuisman tonhuisman commented Aug 22, 2023

Rules where quite rigorously stripped from comments, interfering with url's and // when used in strings.
Features:

  • Don't cut off url's like http://myserver.local:8080/some/path after the colon
  • Don't cut off quoted strings that contain //, the slashes are ignored when between matching quotes

@uzi18
Copy link
Contributor

uzi18 commented Aug 22, 2023

I see here regex usefull

@TD-er
Copy link
Member

TD-er commented Aug 22, 2023

But is parsing regex fast?
At least the required library for it isn't smaller, but it could be used for more parts in the code, so the extra bytes for it may be worth it as other code could be simplified.
However I don't think it will be faster.

@uzi18
Copy link
Contributor

uzi18 commented Aug 22, 2023

@TD-er check first if it is not already included (by any libs?). Regex is always slower.
Fought it is feature to remove comments by one click ;)
As coders know code without comments are headache

@TD-er
Copy link
Member

TD-er commented Aug 22, 2023

I'm not that fluent in regex, so not sure if reading a regex will be done in less time than my internal grey mass C++ parser needs :)

@uzi18
Copy link
Contributor

uzi18 commented Aug 22, 2023

There are some pages to test it over internet.

@tonhuisman
Copy link
Contributor Author

tonhuisman commented Aug 23, 2023

While I have quite some experience with regular expressions, this one is a bit complex, because of the 3 quotes that can be used to delimit a value, and have to match start/end to apply, within where the // is to be ignored. Having a dual character comment start isn't helping also.
Just for giggles I'll try to create one Gave up after 10 minutes, way too complex to match the start/end quotes.
My go-to site for validating (and explaining) a regex is https://regex101.com

@uzi18
Copy link
Contributor

uzi18 commented Aug 23, 2023

@tonhuisman maybe you could change these quotes temporary with one known just to find position of last // with comment

@tonhuisman
Copy link
Contributor Author

@uzi18 I expect to have nailed it with current code, wouldn't want to complicate it further, and replacing quotes isn't going to help finding the comment-start quicker or easier, IMHO.

@TD-er TD-er merged commit 2b937f0 into letscontrolit:mega Sep 17, 2023
154 checks passed
@tonhuisman tonhuisman deleted the bugfix/Rules-improved-comments-removal branch September 17, 2023 16:19
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.

3 participants