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

Cleanup #14777

Merged
merged 29 commits into from
Aug 28, 2020
Merged

Cleanup #14777

merged 29 commits into from
Aug 28, 2020

Conversation

Clyybber
Copy link
Contributor

No description provided.

compiler/wordrecg.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member

timotheecour commented Jun 24, 2020

/cc @Araq I've reviewed everything, looks excellent! feel free to review but I've already done a thorough pass and LGTM (modulo remaining comments) so feel free to skip this one if you want to save time.

@Clyybber

  • just 1 more question:
    TSpecialWord elements have 2 stringifications:
    via canonPragmaSpelling and via specialWords; in your PR, canonPragmaSpelling overrides specialWords when canonPragmaSpelling differs; was there any need for the non-canonical spellings (eg executeonreload vs executeOnReload for wExecuteOnReload)

  • if possible, add a small summary in PR top message for posterity where you list just a few salient points addressed in this PR (eg: removed lexer.cursor, etc)

  • did you use any special tricks to find deadcode?

@Clyybber
Copy link
Contributor Author

@timotheecour

  • There used to be a point, but it gets checked style-insensitive both ways nowadays, so it doesn't matter.
  • Sure, I can add the commit messages, they should give a good idea of whats been done
  • I simply compiled the compiler and the tools with hint[DefinedButNotUsedX]:on and used git blame and grep to find out if they are really unused (and not just unused inside a when false: or commented out). I also noticed a few things that could be cleaned up while looking at the lexer/parser.

@Araq
Copy link
Member

Araq commented Jun 24, 2020

Assignments like ' result = "" ' will become mandatory, we really want definite assignment analysis like C#, so don't remove them. And sorry but I also don't like that you removed my lookup tables. $enumValue should be a debug helper, and say eg. tkIf and the token's display name is "if". Please don't conflate them, the ship has sailed and $ is for debugging.

@timotheecour
Copy link
Member

timotheecour commented Jun 24, 2020

Assignments like ' result = "" ' will become mandatory

until something like that happens, it doesn't make sense to have assignments in a some parts of the code code and not in others parts (other parts of stdlib, or, more importantly, all the third party code ever written that relies on result being assigned. In any case for something like that to happen, there would need to be an RFC + a tool to help with such edits (otherwise it's gonna be really painful), so that tool would be used to fix those anyways.

I also don't like that you removed my lookup tables. $enumValue should be a debug helper

tkIf => if is more than just for debugging, it's the canonical string used in parser, error messages etc. In fact we only use the canonical form for those enums ("if"), not the raw form ("tkIf"); at the very least the canonical form is the one that's used most of the time. This PR makes things DRY and simpler to read/write/maintain.

What we can add (and I can volunteer an implementation) is a way to get the native string for an enum:

type Foo = enum
  f1 = "myF1"
  f2 = "myF2" 

import std/typetraits
const f = f1
static: doAssert $f == "myF1"
static: doAssert f.nativeStr == "f1"

@Araq
Copy link
Member

Araq commented Jun 24, 2020

until something like that happens, it doesn't make sense to have assignments in a some parts of the code code and not in others parts (other parts of stdlib, or, more importantly, all the third party code ever written that relies on result being assigned.

Well the warning already exists and I'll enable it for the compiler. And I've already begun to patch the stdlib. Since the warning already exists, it doesn't need an RFC process for the time being, the compiler's codebase can diverge from the historical default settings.

I don't like the enumValue = "string repr" pattern and you won't convince me, differences in taste exist. So let me please have it my way.

@timotheecour
Copy link
Member

timotheecour commented Jun 24, 2020

I don't like the enumValue = "string repr" pattern and you won't convince me, differences in taste exist. So let me please have it my way.

starting from first principles, a DRY solution is better. It doesn't have to be enumValue = "string repr" but can be a library solution. Can we then reconsider #14008 (with whatever adjustments needed) to be part of stdlib instead of fusion, it's a very common pattern (including for compiler code) with enums and generalizes enumValue = "string repr" to arbitrary values/tuples to include other metadata: I've already shown several use cases in that PR and have more in mind. It solves the DRY aspect and also doesn't bake in strings with enumValue = "string repr".

enums with holes are getting fixed.

"enum with holes" in the title was just contextual, it's really not specific to enum with holes if you follow that PR; it's really about providing "enum map" with the simplest possible syntax and minimal overhead.

@Araq
Copy link
Member

Araq commented Jun 25, 2020

Can we then reconsider #14008

Yeah, that's a nice compromise. Bring it on please!

@timotheecour
Copy link
Member

done, I suggest putting this PR on temporary hold pending #14008

@stisa
Copy link
Contributor

stisa commented Aug 28, 2020

I think the module compiler/forloops is also dead code, or at least I can't find it used anywhere, so you may want to delete it in this PR?

compiler/btrees.nim Outdated Show resolved Hide resolved
@Clyybber Clyybber requested a review from Araq August 28, 2020 18:43
@Araq Araq merged commit 13e659c into nim-lang:devel Aug 28, 2020
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
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