FabricMC / tiny-remapper

Tiny JAR remapping tool.
GNU Lesser General Public License v3.0
110 stars 64 forks source link

Local variable name generation ideas #22

Open natanfudge opened 4 years ago

natanfudge commented 4 years ago

Feel free to add more or tick checkboxes.

2xsaiko commented 4 years ago

Map<K, V>kToV / kToVMap

Pyrofab commented 4 years ago

Iteratorit maybe

natanfudge commented 4 years ago

Iterator → it maybe

That just seems less readable than iterator?

2xsaiko commented 4 years ago

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

Runemoro commented 4 years ago

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

2xsaiko commented 4 years ago

ItemPlacementContext, ItemUsageContext, *Contextctx BlockPospos BlockStatestate PlayerEntityplayer Directiondir BlockViewview

Pyrofab commented 4 years ago

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

liach commented 4 years ago

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

natanfudge commented 4 years ago

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

modmuss50 commented 4 years ago

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

natanfudge commented 4 years ago

Gonna need lv remapping first

modmuss50 commented 4 years ago

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

sfPlayer1 commented 4 years ago

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.

2xsaiko commented 4 years ago
* 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

Runemoro commented 4 years ago

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?

natanfudge commented 4 years ago

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

natanfudge commented 4 years ago

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

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

Runemoro commented 4 years ago

@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.

natanfudge commented 4 years ago

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.

Runemoro commented 4 years ago

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.

natanfudge commented 4 years ago

Is weave still alive?

Runemoro commented 4 years ago

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

natanfudge commented 4 years ago

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.

Runemoro commented 4 years ago

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

natanfudge commented 4 years ago

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

Runemoro commented 4 years ago

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).

sfPlayer1 commented 4 years ago

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.

RedstoneParadox commented 4 years ago

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

natanfudge commented 4 years ago

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

Runemoro commented 4 years ago

Suggestions for short names:

Runemoro commented 4 years ago

Map<K, V> → kToV / kToVMap

For maps, it should just be vs if there is only one map with value of type V. If there's several, it can be vsByK.

Marcono1234 commented 4 years ago

Tiny Remapper currently increments the letters for primitive local variables (except for booleans), e.g. (int ?, int ?) would become (int i, int j). However, in my opinion this behavior is not so great because it often creates non-descriptive or irritating variables names if there are multiple variables, e.g.:
(char ?, char ?, double ?, double ?, float ?) -> (char c, char d, double e, double f, float g)

It would probably better to just number them, e.g.:
(char ?, char ?, double ?, double ?, float ?) -> (char c, char c2, double d, double d2, float f)

sfPlayer1 commented 4 years ago

I found the distinct characters much more readable than incrementing the suffix after having experimented with both, could be made configurable though.

coderbot16 commented 4 years ago

I think that after a certain number of local variables, numbering is more readable than incrementing letters. I think both approaches could be appropriate.