Update KaitaiStruct compiler and runtime to 0.9

[Mingun]

No description provided.

[generalmimon]

@Mingun Can you now rebase the to-0.9 branch on top of master to see if the CI will pass?

[Mingun]

I also thought that now there is no need to turn off auto-reading, because we still call the _read() just after construction. Previously, it was turned off automatically when the debug fields were turned on, now this can be avoided

[generalmimon]

Previously, it was turned off automatically when the debug fields were turned on, now this can be avoided

I don't think it should be done just because it can be done now. What do you gain by doing it - saving 2 lines? Why are you arbitrarily replacing something that worked by something that maybe works the same? Besides, I think that disabling autoRead still makes sense for visualizers, because you could call _read() from a try..catch block so that even if it fails, you can still dump the partial struct.

Moreover, this is again something that is not related to the PR topic.

[Mingun]

Maybe when the utility will be rewritten and you will actually call this in the try catch block in several places. But for now in called just once after construct so I do not see why we should keep it for now.

Progress in any case requires that kaitai-io/kaitai_struct_compiler#197 and related will be merged -- without that it is impossible to move ahead because utility cannot correctly handle even trivial cases. But judging the speed of how PRs are merged I'm predict that it will be happened not early than after ten years. At that time everything can changed.

What do you gain by doing it - saving 2 lines?

Using reflection is ugly. Compiler won't be able to tell you if you're wrong. So if you can not use it it is better to not use it. Gentleman's agreement.

Why are you arbitrarily replacing something that worked by something that maybe works the same?

How do you know it works? "That worked" debug = true in version 0.8, why do you have no doubt that autoRead=false + readStoredPos=true works, but doubt that autoRead=true + readStoredPos=true works in 0.9?

Moreover, this is again something that is not related to the PR topic.

This PR about upgrading version of the dependency which API is changed. New code shows the same behavior (I've tested that) using the new API so I do not see why you can say that one change is related to the PR and other is not. I do not see the principal difference.

[generalmimon]

"That worked" debug = true in version 0.8, why do you have no doubt that autoRead=false + readStoredPos=true works, but doubt that autoRead=true + readStoredPos=true works in 0.9?

I have no doubt that autoRead=false + readStoredPos=true works because that's exactly what the debug option did in 0.8 (and the --debug CLI flag still does, see JavaMain.scala:164).

autoRead=true + readStoredPos=true is a new combination that had been never possible before 0.9, something that we do not even have a single test for, and we only assume it works.

I do not see why you can say that one change is related to the PR and other is not.

Because the old instantiation of RuntimeConfig set debug param to true, which in fact meant autoRead=false + readStoresPos=true, and you're now doing autoRead=falsetrue + readStoresPos=true. This change is inequivalent, does principally a different thing than the previous version of the code for the 0.8 compiler. You cannot say that switching to 0.9 has forced you to do this, because it has not - therefore it's an unrelated, separate change, it has little to do with this PR, and it can be better done in another. That will keep the individual PRs small and easy to review.

But judging the speed of how PRs are merged I'm predict that it will be happened not early than after ten years.

What exactly is the intended effect of this ironic remark? If you want to speed up the process, do not pack multiple changes into one pull request if they could perfectly work independently. By including a slightly complicated or controversial change into a pull request which would be otherwise ready to be merged by itself without that change, you are risking that the change (or worse several such changes) will prevent merging of the entire PR. When these changes are in their separate PRs, we can all focus to each one of them, which every single pull request focused and easier to review. Perhaps you think that a lower number of pull requests is better, so "if I want to commit several changes that are loosely connected, I'll do that in a single PR", but it's the exact opposite.

If a pull request takes long to merge, it may mean that it's hard (or at least not easy) to properly review it.

[Mingun]

I have no doubt that autoRead=false + readStoredPos=true works because that's exactly what the debug option did in 0.8

You say so because you known the source code. This is absolutely doesn't follow from the API.

Because the old instantiation of RuntimeConfig set debug param to true, which in fact meant autoRead=false + readStoresPos=true

Not exactly true. The old code did debug=true + manual _read() and that is the same as autoRead=false + readStoresPos=true + manual _read() which is fully equivalent of the autoRead=true + readStoresPos=true. When upgrading the old code you can choose the any variant. You would choose the first, I chosen the second.

The old API just didn't have possibility to avoid manual _read()s and have position information at the same time, but new has. Because call of the _read() was done exactly once in the code base I bet, that is was done just as an workaround for that behavior, which is not required anymore. If you fear that this change broke some hypothetical ability to partial parse -- no, it didn't, because the program has no such ability.

If you want to speed up the process, do not pack multiple changes into one pull request if they could perfectly work independently.

I hope, I demonstrate that both opinions, my and your, take to make sense. We just have a different view of the _read() call. For me it seems as a workaround for the unintended implications of the debug=true call, for you it is unfinished attempt to make a parser error-friendly.

If a pull request takes long to merge, it may mean that it's hard (or at least not easy) to properly review it.

I understand this, but what I cannot understand is why it is impossible to answer at least somehow questions about the status (which I've asked several times in my PRs). The impression is not very good, it seems that no one needs this project at all.

What exactly is the intended effect of this ironic remark?

Maybe to show that development just stopped and it's pretty sad :(. In this light, fears of breaking something what is already broken look especially ironic, but perhaps this flaw is simply unknown to maintainers (although I tried to explain what the problem), because them do not use the program. Perhaps to show that it is worth thinking about whether everything goes as it should go. Push someone to action. Many things.

[generalmimon]

I understand this, but what I cannot understand is why it is impossible to answer at least somehow questions about the status

@Mingun What status would you like to know?

[generalmimon]

BTW, in hindsight, this was a pretty useless bikeshedding thread and wasted time of both of us. Sometimes I should resist the urge to deal with insignificant trifles.

[Mingun]

@Mingun What status would you like to know?

I would like to get answers to my questings in my PRs, especially here (and related) and here (this one turned into an absurd situation when the corresponding runtime changes merged, but changes in the compiler is not, which means that runtime changes is not only pointless, but also wrongly documented)

[generalmimon]

First, this is not true - only readStoresPos controls this. autoRead controls just whether the this._read() call should be called from the constructors of generated classes. See kaitai-io/kaitai_struct#332 - old debug parameter did both these things, but in 0.9 they can be controlled separately (at least from the JVM environment).

Second, _attrStart/_attrEnd/_arrStart/_arrEnd is very repetitive and it may be hard to spot the difference between attr and attrr at first sight. Personally I like to use glob syntax when there's a need to "list" all identifiers with a same pattern, this would be _{attr,arr}{Start,End} here - but agreed, it might not be obvious to everyone what that means.

The phrase "required for existing ... fields" also sounds unnatural to me, I would use kaitai-struct-compiler --help as a inspiration and write:

Suggested change

	false,// autoRead - required for existing _attrStart/_attrEnd/_arrStart/_arrEnd fields
	true, // readStoresPos - required for existing _attrStart/_attrEnd/_arrStart/_arrEnd fields
	false, // autoRead - whether to automatically call `_read` in constructor
	true, // readStoresPos - `_read` remembers attribute positions in stream

The actual field names where the position info will be stored are not so important anyway.

[Mingun]

The actual field names where the position info will be stored are not so important anyway.

This comment was added to give the reader an information that this setting controls the presence of fields that accessed via Reflection in DebugAids. I've reword it slightly.

[generalmimon]

I don't like this entire invocation, but I understand that Java does not support default parameter values, so it cannot be made much better. Assigning null to everything can cause errors if there would be an attempt to use the config for some other compiler class than JavaCompiler, but so far, so good.

[Mingun]

The config is intended to use with the JavaCompiler only, there is not supposed to call it for other languages, we need it is only for getting the Java code for compilation.

[generalmimon]

The config is intended to use with the JavaCompiler only, there is not supposed to call it for other languages, we need it is only for getting the Java code for compilation.

I understand, and the actual form looks kind of OK given the circumstances, but there is a hidden assumption that the RuntimeConfig object will tolerate being in partially-invalid state (for example in Scala, it would not be possible to pass null to a param with type String or CppRuntimeConfig).

We know this is not a problem because we can inspect the compiler code and find out that the JavaCompiler only reads properties config.{java,readStoresPos,autoRead}, Main.importAndPrecompile() only reads config.opaqueTypes and Main.compile() only passes the config to JavaCompiler (evenvutally overriding debug if /meta/ks-debug is set). But I don't like that these assumptions are hidden. If you could write a short comment that such initialized RuntimeConfig can only be used to initialize JavaCompiler, I would be more happy about that.

In an ideal world, Java would support default parameter values and named parameters, so you could do just new RuntimeConfig(autoRead = false, readStoresPos = true, opaqueTypes = true, java = new JavaRuntimeConfig(...)), not care about other values and they would be filled with sensible defaults set in RuntimeConfig.scala:91-103 - so it would be safe. But this is unfortunately not the case.

[generalmimon]

Thanks!