Skip to content

Fix analysis errors related to non-concrete type usage#1547

Merged
tonyqus merged 1 commit into
nissl-lab:masterfrom
lahma:use-concrete-types
Apr 21, 2025
Merged

Fix analysis errors related to non-concrete type usage#1547
tonyqus merged 1 commit into
nissl-lab:masterfrom
lahma:use-concrete-types

Conversation

@lahma
Copy link
Copy Markdown
Collaborator

@lahma lahma commented Apr 19, 2025

It's better to return and use concrete types instead of abstract classes and interfaces as it performs better. Removed analysis ignore and fixed issues. Changes target non-public API so there should be no breaking changes to end users.

I also changed some iterator/enumerator usage more idiomatic foreach in places where analysis complained about using interface to iterate.

Comment thread main/HPSF/Variant.cs
public static int GetVariantLength(long variantType)
{
long key = (int)variantType;
if (numberToLength.Contains(key))
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

compared this to Java version and there was actually a bug here, it should have been not contains -> -2

@lahma lahma marked this pull request as ready for review April 19, 2025 07:33
@lahma
Copy link
Copy Markdown
Collaborator Author

lahma commented Apr 19, 2025

Hmm.. did I accidentally also fix the Windows build 🤔

@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Apr 21, 2025

I'm not sure if the Windows build issue is solved. Let's see

@tonyqus tonyqus added this to the NPOI 2.7.4 milestone Apr 21, 2025
@tonyqus tonyqus merged commit 55a3dbf into nissl-lab:master Apr 21, 2025
3 checks passed
@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Apr 21, 2025

Unforunately 👯‍♂️

https://github.com/nissl-lab/npoi/actions/runs/14582014649

@lahma lahma deleted the use-concrete-types branch April 22, 2025 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants