tiny-remapper icon indicating copy to clipboard operation
tiny-remapper copied to clipboard

Local variable name generation ideas

Open natanfudge opened this issue 5 years ago • 34 comments

  • [ ] Give types that implement Collection a plural name (In addition to arrays)
  • [ ] Name boolean variables flag, flag2, etc instead of bl, I think that's more readable.
  • [ ] For primitive arrays, use ints, and floats, instead of is and fs, etc

Feel free to add more or tick checkboxes.

natanfudge avatar Nov 10 '19 16:11 natanfudge

Map<K, V>kToV / kToVMap

2xsaiko avatar Nov 10 '19 19:11 2xsaiko

Iteratorit maybe

Pyrofab avatar Nov 10 '19 21:11 Pyrofab

Iterator → it maybe

That just seems less readable than iterator?

natanfudge avatar Nov 10 '19 21:11 natanfudge

I'd go for iter. Everyone seems to use that, and it's readable.

2xsaiko avatar Nov 10 '19 21:11 2xsaiko

If a variable contains the result of a method named getXyz, it should be named xyz.

Runemoro avatar Nov 10 '19 23:11 Runemoro

ItemPlacementContext, ItemUsageContext, *Contextctx BlockPospos BlockStatestate PlayerEntityplayer Directiondir BlockViewview

2xsaiko avatar Nov 12 '19 17:11 2xsaiko

Building upon the PlayerEntity suggestion, (.*)Entity -> $1

Pyrofab avatar Nov 12 '19 17:11 Pyrofab

We should just allow the remapper to accept a local variable name suggestor

liach avatar Nov 12 '19 18:11 liach

ItemPlacementContext, ItemUsageContext, *Context → ctx BlockPos → pos BlockState → state PlayerEntity → player Direction → dir BlockView → view

  • I think context is fine
  • yes
  • yes
  • no, dir is directory
  • yes

natanfudge avatar Nov 12 '19 19:11 natanfudge

I think expanding the auto name generator to include these in a yarn dist is the best way to go about this?

modmuss50 avatar Nov 12 '19 19:11 modmuss50

Gonna need lv remapping first

natanfudge avatar Nov 12 '19 19:11 natanfudge

Ah yeah, didnt read the local bit. Could still do it for params possibly? might not be needed tho

modmuss50 avatar Nov 12 '19 19:11 modmuss50

Tiny remapper's name generation should remain independent of MC, so hardcoding rules for its typical classes is out - only important Java ones may receive some. The same applies to anything that can't be inferred without simple analysis, i.e. needing more than the descriptor+signature of the variable to be named.

You can still keep submitting ideas that are more extensive, we can implement more comprehensive schemes elsewhere in the tooling.

sfPlayer1 avatar Nov 12 '19 19:11 sfPlayer1

* I think `context` is fine

I mean sure it's fine, but you want that name to not get in the way since most if not all of these are a container for other values whose names are the actually important part you want to look at when you're reading the code.

* no, `dir` is directory

Wouldn't be the first time an abbreviation could mean two different things based on context. And in most cases you're not going to be handling any directories in the same places as handling Directions so they won't conflict.

Strings (not sure if this already happens) StringBuildersb ? extends Exceptione

2xsaiko avatar Nov 12 '19 20:11 2xsaiko

Tiny remapper's name generation should remain independent of MC, so hardcoding rules for its typical classes is out

Can't the tiny format be extended to include a "default variable name" property for class mappings?

Runemoro avatar Nov 12 '19 20:11 Runemoro

Why is there a need for a default variable name? Whoever creates the mappings file can:

  • Put the name if it was inputted by a human
  • Otherwise, if this name is special (like *context), put the desired name (context in this case)
  • Otherwise, leave empty and let tiny-remapper handle it.

natanfudge avatar Nov 12 '19 20:11 natanfudge

String → s (not sure if this already happens) StringBuilder → sb ? extends Exception → e

I'm really against such short names in general...

natanfudge avatar Nov 12 '19 20:11 natanfudge

@natanfudge I'm not sure what you're saying... therealfarfetchd suggested rules for automatically generating simpler names for certain classes, like BlockPos -> pos, and Player replied that they can't be hardcoded into tiny-remapper. I then suggested that they can be part of the mapping files.

Runemoro avatar Nov 12 '19 20:11 Runemoro

I think what you suggested, is to extend the tiny format, with new syntax. I replied that what Player wants is achievable using the existing format.

natanfudge avatar Nov 12 '19 20:11 natanfudge

is achievable using the existing format.

It isn't. With the existing format, we can only map things individually. Here we want a rule for all BlockPos variables to be named pos (plus a number when there's more than one).

Unless you're suggesting moving the name generation to when yarn is built, but in that case this issue should be for yarn, and the logic should be implemented in either stitch or Enigma.

Runemoro avatar Nov 12 '19 21:11 Runemoro

Is weave still alive?

natanfudge avatar Nov 12 '19 21:11 natanfudge

No, weave has been merged into Enigma and is no longer necessary.

Runemoro avatar Nov 12 '19 21:11 Runemoro

The original idea of this issue was about generic things (See the opening comment), I agree that minecraft-specific stuff can be moved to yarn.

natanfudge avatar Nov 12 '19 21:11 natanfudge

It doesn't make sense to have variable name generation both in yarn and tiny-remapper...

Runemoro avatar Nov 12 '19 21:11 Runemoro

Well, variable name generation is already partially implemented in tiny-remapper.

natanfudge avatar Nov 12 '19 21:11 natanfudge

And it can be kept there. But we'll need a way to provide the custom rules to tiny-remapper (for example, by specifying the rules in the tiny files).

Runemoro avatar Nov 12 '19 21:11 Runemoro

I like the idea of adding the default variable name, not sure how it'd interact with enigma / manual mapping authoring though. Some external tool with the knowledge+analysis capabilities of e.g. matcher would still have to handle complex cases like inferring names from getter returns etc - that would apply to field names as well.

sfPlayer1 avatar Nov 12 '19 21:11 sfPlayer1

Gonna plus one not renaming int arrays to is as that would surely cause issue in Kotlin where is is a keyword.

RedstoneParadox avatar Nov 12 '19 21:11 RedstoneParadox

This is about parameters/variables, which you can't interact with from a Kotlin mod.

natanfudge avatar Nov 12 '19 21:11 natanfudge

Suggestions for short names:

  • String -> s
  • Object -> o
  • Character, char, result of String.codePointAt -> c
  • Byte, byte -> b
  • Integer, int, Short, short, Long, long -> i, j, k, n, m, ii, jj, ...
  • Float, float, Double, double -> a, b, c

Runemoro avatar Nov 12 '19 22:11 Runemoro