Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added missing FileVersionInfo functionality #906

Open
wants to merge 6 commits into
base: 0.3
Choose a base branch
from

Conversation

Sorenon
Copy link

@Sorenon Sorenon commented Jun 11, 2020

Added missing functions to winver.rs
Reordered winver.rs to match the order of the original header
Replaced explicit pointers in winver.rs with their concise winapi equivalents

Created versrc.rs from versrc.h and populated it with the constants and struct type

Added missing functions to winver.rs
Reordered winver.rs to match the order of the original header
Replaced explicit pointers in winver.rs with their concise winapi equivalents

Created versrc.rs and populated it with its constants and a struct type
@Sorenon Sorenon changed the title Added missing FileVersionAPI functionality Added missing FileVersionInfo functionality Jun 11, 2020
@retep998 retep998 added the waiting on review Waiting for a reviewer to review the PR label Jun 15, 2020
Copy link

@roblabla roblabla left a comment

Choose a reason for hiding this comment

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

Needs a few fixes.

  • Organization of code

    • All definitions go into the source file that directly maps to the header the definition is from:
      • ⚠️ versrc doesn't match header file name verrsrc.h.
    • Definitions are defined in the same order as they are in the original header: ✔️
  • Extern functions

    • The calling convention specified should be the one for 32bit: ✔️
  • Constants

    • Names are correct ✔️
    • Values are correct ⚠️ one small issue.
    • Types are correct: All the constants are meant to be used in the VS_FIXEDFILEINFO, which is all DWORD: ✔️
  • Structs

    • Wrapped in the STRUCT! macro, matches C structure: ✔️

Cargo.toml Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
src/um/mod.rs Outdated Show resolved Hide resolved
src/um/versrc.rs Outdated Show resolved Hide resolved
src/um/versrc.rs Outdated
@@ -0,0 +1,105 @@
// Licensed under the Apache License, Version 2.0

Choose a reason for hiding this comment

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

File should be renamed verrsrc.rs

Sorenon and others added 5 commits December 2, 2020 13:13
Co-authored-by: Robin Lambertz <[email protected]>
Co-authored-by: Robin Lambertz <[email protected]>
Co-authored-by: Robin Lambertz <[email protected]>
Co-authored-by: Robin Lambertz <[email protected]>
Copy link

@roblabla roblabla left a comment

Choose a reason for hiding this comment

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

LGTM 👍

All problems from previous review fixed!

@roblabla roblabla mentioned this pull request Dec 4, 2020
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting on review Waiting for a reviewer to review the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants