Skip to content

Conversation

@LogicAndTrick
Copy link
Contributor

I started this a few days ago, and I just noticed that you were working on it as well so hopefully this can save you some time. I've compiled the test cases and the outputs all build using .NET Core. I haven't tried running them, though.

I've made some changes so that the result fits into the .NET naming standards and it takes advantage of some of the C#/.NET features that Java doesn't have.

  • Added a .NET namespace config option (but defaults to Kaitai if not specified)
  • Using .NET naming standards (public members use PascalCase, private members use camelCase)
    • Properties can't have the same name as classes, so the public getters are named Get{Thing}() instead of just {Thing}()
  • I've done a (hopefully) complete implementation of the KaitaiStream class in my runtime fork at LogicAndTrick/kaitai_struct_csharp_runtime, let me know if you would like a PR for the runtime.
    • The runtime stream is based off .NET's BinaryReader class which has implementations for little-endian types, so only the BE methods need to be implemented.
    • As a bonus the BinaryReader already has ReadSingle and ReadDouble (and others), so floating point support comes free.
  • All the classes are marked as partial so users can add methods and properties to the class without touching the generated file

Apologies if this conflicts with anything you're working on! I've never used Scala before, so hopefully I didn't do anything wrong.

out.puts(s"{")
out.inc
out.puts(s"return new ${type2class(name)}(new $kstreamName(fileName));")
out.puts(s"return new ${type2class(name)}(new KaitaiStream(fileName));")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the idea of changing it back to literal KaitaiStream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may have been caused because I started from the Java compiler originally. I must have missed a few changes when I was matching it back to what you had. There's a couple of others with the renaming of the camelCase/member naming functions, I'll get them cleaned up.

@GreyCat GreyCat self-assigned this Jul 17, 2016
@GreyCat
Copy link
Member

GreyCat commented Jul 17, 2016

Thank you so much for you hard work here, it's certainly really helpful :)

I've reviewed and commented all the changes you've sent. Could you fix / respond to stuff I've mentioned and update this pull request, so I could apply it right away?

I'm currently working on a testing system for C#, and actually it's almost finished. I'll try to deploy it to Travis soon enough, so we can get things rolling in C# department, with proper CI and progress measurements at http://kaitai.io/ci/

As a side note, a little advice for the future commits: could you please send them in more distinct, separate commits, so I could have applied some right away?

@LogicAndTrick
Copy link
Contributor Author

No problems, thanks for the quick review. I'll get those changes in as soon as I can.

@GreyCat
Copy link
Member

GreyCat commented Jul 18, 2016

I've done a (hopefully) complete implementation of the KaitaiStream class in my runtime fork at LogicAndTrick/kaitai_struct_csharp_runtime, let me know if you would like a PR for the runtime.

I've already reviewed and I agree with most of it, except for GetXXX stuff and renaming of _io to stream (_io should probably become m_Io, as per MS / .NET core codestyle). But I'm definitely in no position to judge any C# code, you people obviously know it much better than I do. Please do the PR, so other people interested in the C# support could review it too (i.e. @indrora).

@LogicAndTrick
Copy link
Contributor Author

I'm working on this now, the naming of things is going to get a bit messy because of the conflicts. Java allows for something like this:

class Test {
  private String example;
  public String example() { return example; }
}

Whereas this will result in a compiler error in C# due to duplicate declarations of the same name. I'm going with this alternative for now:

class Test {
  private String _example;
  public String example { get { return _example; } }
}

This doesn't suit the naming standard (public property should be PascalCase), but then it breaks in the case of an inner class...

class Test {
  public class Example { ... }
  private Example _example;
  public Example Example { get { return _example; } } // <- `Example` is already defined in class
}

The other option is to name inner classes something else, such as _Example or Example_Inner. But that will also deviate from the standard. Which do you think would be the better option? Break C# naming standards by using camelCase for getters, or change the name of inner classes?

Or should I use PascalCase and leave the test cases broken to be fixed later?

@LogicAndTrick
Copy link
Contributor Author

Closing this - Created PR #2 instead.

@LogicAndTrick LogicAndTrick deleted the csharp branch July 25, 2016 12:11
generalmimon added a commit that referenced this pull request Apr 7, 2024
Fixes kaitai-io/kaitai_struct#1086

Until now, top-level types were treated the same as opaque types, which
had the unintended consequence that arguments passed to top-level types
were not validated against the number and data types of declared
parameters.

This commit fixes the `formats_err` tests added in
kaitai-io/kaitai_struct_tests@9d518e6 -
more specifically, these failures in `sbt test` output are now fixed:

```
- params_call_bad_type_top_import *** FAILED ***
  []
    did not equal
  [params_call_bad_type_top_import.ksy: /seq/0/type:
  	error: can't pass argument #1 of type CalcFloatType into parameter `has_trailer` of type CalcBooleanType
  ] (SimpleMatchers.scala:34)
- params_call_bad_type_top_local *** FAILED ***
  []
    did not equal
  [params_call_bad_type_top_local.ksy: /seq/0/type:
  	error: can't pass argument #1 of type CalcFloatType into parameter `has_trailer` of type CalcBooleanType
  ] (SimpleMatchers.scala:34)
- params_call_too_many_top_import *** FAILED ***
  []
    did not equal
  [params_call_too_many_top_import.ksy: /seq/0/type:
  	error: parameter count mismatch: 2 declared, but 3 used
  ] (SimpleMatchers.scala:34)
- params_call_too_many_top_local *** FAILED ***
  []
    did not equal
  [params_call_too_many_top_local.ksy: /seq/0/type:
  	error: parameter count mismatch: 2 declared, but 3 used
  ] (SimpleMatchers.scala:34)
```
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.

2 participants