Protobuf-net Gendarme Errors

1
votes

Recently came across protobuf-net, awesome library.

I ran it through gendarme and it came up with many performance notifications, e.g:

Target: System.Int32 ProtoBuf.ProtoReader::ReadFieldHeader() Assembly: protobuf-net, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null

Severity: High Confidence: High Source: debugging symbols unavailable, IL offset 0x0055 Details: Type 'System.Int32' is being boxed.

If anyone has experience with gendarme how important are these sort of notifications and is this something I could possibly contribute back to protobuf-net by attempting to clean up any Severity: High issues?

1
c#protobuf-netgendarme
Guess it depends on what you are really trying to do. Marc makes the source available, would it not be more effective if you are that concerned that you review the source? you can always get the PDB from the download/NuGet as well ..yourproject\packages\protobuf-net.2.0.0.621\lib\net40 there are many tests in the project as well. – Paul Farry
I can take a look in a day or two (just got back from a long flight) - but: it could be a false-positive: protobuf-net actually exists as a double code-base: a reflection model and a meta-programming model. It uses the best it can, which usually means the meta-programming model. It could be that it is complaining about the reflection model (which is less efficient by necessity). But: I'll take a look. Btw,the PDBs are fully available - it should have access to the debugging symbols – Marc Gravell
For info, I've been through these; none of them were particularly noticeable, but I've done some tidying up to make gendarme happier - but frankly they were false-positives really. The only thing of note it caught was one place where I had the wrong #if markers, meaning the custom exception wasn't binaryformatter-serializable. Not exactly a critical point. – Marc Gravell

1 Answers

1
votes

The specific error you cite comes from:

if(fieldNumber < 1) throw new ProtoException(
    "Invalid field in source data: " + fieldNumber);

so, yes technically this is an unnecessary box - HOWEVER, it is an extreme edge case that is not worth caring about (if you get that exception, you have bigger problems than the box). It could be fixed if it is causing you concern, though.

It is also likely that some further errors will be being raised from the reflection implementation - which would be misleading because in most scenarios that isn't actually used (the code has both a reflection model and a meta-programming model).

I will aim to have a look at what gendarme says next week: update - done - note that most of this was basically "busy work" - it didn't really change anything important, other than it made Gendarme happy.