GrammaTech / gtirb

Intermediate Representation for Binary analysis and transformation
https://grammatech.github.io/gtirb/
Other
305 stars 36 forks source link

FIx compile warnings about unused-variables & Wrap templates into namespace #65

Open 5c4lar opened 5 months ago

5c4lar commented 5 months ago

When using gtirb as a lib, including the header leads to compiler complaining that use of some templates are ambiguous, wrap the templates into namespace should solve the problem.

aeflores commented 4 months ago

Hi @5c4lar, thanks for contributing to gtirb! We require a Contributor License Agreement (CLA) before reviewing and accepting contributions, would you mind signing one?

You can find a copy at https://github.com/GrammaTech/gtirb/blob/master/GrammaTech-CLA-GTIRB.pdf but since you have opened pull requests for pprinter and ddisasm, you might want to sign the following GrammaTech-CLA_ddisasm, gtirb, & gtirb pprinter.pdf which covers the three projects.

5c4lar commented 4 months ago

Hi @aeflores , I've signed the CLA and sent it to CLA@GrammaTech.com. Looking forward to your reply!

adamjseitz commented 2 months ago

Hi @5c4lar thanks for your PR, and apologies for the delay in reviewing this and the related gtirb-pprinter and ddisasm PRs.

One concern we have is that this is a breaking change. Any downstream projects using GTIRB as a library will break until they make changes reference any of the moved declarations via the namespace.

One possible solution to this is do something like:

  1. Move functionality into the GTIRB namespace (what you've already done).
  2. Map the current names into the namespace by default for backwards compatibility, wrapped in an #ifndef:
#ifndef DISABLE_GTIRB_GLOBAL_NAMESPACE_DECLARATIONS

using gtirb::NextPowerOf2;
using gtirb::isPowerOf2_64;
// ...etc

#endif

This would give existing library users time to transition, updating code to access these via the namespace as they are ready, and allow you to immediately define DISABLE_GTIRB_GLOBAL_NAMESPACE_DECLARATIONS to resolve your compiler errors.

What do you think - would this be a suitable solution for you? Do you think this is something you could implement?

5c4lar commented 2 months ago

Hi @adamjseitz . Thank you for your feedback and for considering the implications of the breaking change. I appreciate the detailed explanation and the proposed solution to address the backward compatibility concerns.

I have updated the PR as per your suggestion, moving the functionality into the GTIRB namespace and adding the conditional compilation directives for backward compatibility.

Thank you for your patience and guidance.

jdorn-gt commented 1 week ago

These changes were merged in our internal repo (the GitHub repo is a mirror of the internal one) on August 23. You can see the diff that merged here: https://github.com/GrammaTech/gtirb/compare/4ff067e..3b5766f