Permit trivial TRVE usage in jass input #893
Conversation
There was a problem hiding this comment.
The optimizer is not the only point where variable names might be changed.
- There might be places where we add a scope name. For example a static variable
xin classAwould get a name likeA_xif I remember correctly. - In the translation to Jass names are made unique if they are not. This might also affect global variables.
So I am not sure, if this PR would fix TRVE for all cases. Maybe you can do some tests. It's probably enough for the case of global Jass variables, which should be the most common use of TRVE.
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/TRVEHelper.java
Outdated
Show resolved
Hide resolved
de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/WurstValidator.java
Outdated
Show resolved
Hide resolved
|
Okay I added 2 jass unit tests and did some real world map tests. |
Frotty
changed the title
|
I have been using this impl in w3protect for some time - so far without issues. I would like to merge it at some point. I think we should only allow it in Jass though, to prevent the issues discussed. |
|
@peq anything stopping this from being merged? |
There was a problem hiding this comment.
I guess that some more checks are required for this to work reliably. For example there might be two variables with the same name in Wurst and the compiler will rename one to avoid the conflict.
It would be cleaner to add a special construct to the Im like we did for ExecuteFunc.
However, if you just want this to optimize Gui maps, then you can merge it after the static var is gone.
| public void TriggerRegisterVariableEvent(IlConstHandle trigger, ILconstString varName, IlConstHandle opcode, ILconstReal limitval) { | ||
| TriggerMock triggerMock = (TriggerMock) trigger.getObj(); | ||
| // TODO | ||
| // triggerMock.registerEvent(); |
There was a problem hiding this comment.
I guess this should just be an empty mock, so remove the Todo?
| replacements.add(Pair.create(e, Collections.singletonList(e.getRight()))); | ||
| } | ||
| } else if (e.getLeft() instanceof ImVarArrayAccess) { | ||
| ImVarArrayAccess va = (ImVarArrayAccess) e.getLeft(); | ||
| if (!trans.getReadVariables().contains(va.getVar())) { | ||
| if (!trans.getReadVariables().contains(va.getVar()) && !TRVEHelper.protectedVariables.contains(va.getVar().getName())) { |
There was a problem hiding this comment.
Would it be possible to just count TRVE as a read?
| import java.util.HashSet; | ||
|
|
||
| public class TRVEHelper { | ||
| public static final HashSet<String> protectedVariables = new HashSet<>(); |
There was a problem hiding this comment.
Please don't use static variables with mutable state.
There was a problem hiding this comment.
You didn't answer last time. I can't add it as variable to the ImTranslator because I need it in the validator. You want it in the outer scope of that?
| WLogger.info("keep: " + varName.getValS()); | ||
| return; | ||
| } else if (e.getArgs().get(1) instanceof ExprVarAccess) { | ||
| // Check if this is a two line hook... thanks Bribe |
There was a problem hiding this comment.
Is this for a specific bj function? Maybe add a comment...
There was a problem hiding this comment.
This is for single nested calls iirc to detect them as statically resolvable. bribe used to use it in his damage engine.
function myfunc (with same params and return typ)
TriggerRegisterVariableEvent(...)
A bit bad solution maybe :D
did this for w3p, should work?
Needs some fixing and a test for compiler use.