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

Modernization refactorization #123

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jtsylve
Copy link
Contributor

@jtsylve jtsylve commented Jun 25, 2019

Since we've discussed bumping the major version number on the next release, I figured this was a good time to further break the public API a bit (your big refactor in March already broke it anyway).

This PR does a significant amount of refactoring around modernization of the codebase. It tries to accomplish a number of things:

  1. When possible, all manual memory management has been removed. A few edge cases where that wasn't possible were documented.

  2. Due to [1] the public C++ API to add command line arguments to imagers has been changed.

  3. In order to facilitate [1] and to improve type safety, the way object attributes are stored and queried has been significantly changed. This seems to improve usability quite a bit and now queried attribute values can be queried for type, compared against other queried values and AFF4 objects, and are type safe via use of a variant instead of raw pointers to inherited objects.

NOTE

This PR does add a new dependency on Google's abseil libraries, so the docker file and build instructions will need to be updated (not really my strong suit). The addition of abseil is nice, because the lightweight library gives us access to many C++14 and C++17 standard library enhancements without having to raise the minimum requirement of the project from C++11. If compiled against a later standard which already has these features (see PR #122), abseil quietly defaults to the standard library implementation instead of their own.

@scudette
Copy link
Collaborator

scudette commented Jul 8, 2019

I am having some trouble building this PR. I am probably doing something wrong. Any ideas?

In file included from rdf.cc:17:
../aff4/attributes.h:87:9: error: declaration of ‘RDFBytes’ [-fpermissive]
         RDFBytes,
         ^~~~~~~~
In file included from ../aff4/lexicon.h:33,
                 from ../aff4/aff4_base.h:55,
                 from rdf.cc:16:
../aff4/rdf.h:97:7: error: changes meaning of ‘RDFBytes’ from ‘class aff4::RDFBytes’ [-fpermissive]
 class RDFBytes: public RDFValue {
       ^~~~~~~~

@jtsylve
Copy link
Contributor Author

jtsylve commented Jul 9, 2019

That's weird. Since the enum is declared inside of a class, it shouldn't violate the one definition rule.

The spec even states

A class name (9.1) or enumeration name (7.2) can be hidden by the name of an object, function, or enumerator declared in the same scope. If a class or enumeration name and an object, function, or enumerator are declared in the same scope (in any order) with the same name, the class or enumeration name is hidden wherever the object, function, or enumerator name is visible

It also compiles for me. What compiler are you using that gives this error?

@jtsylve
Copy link
Contributor Author

jtsylve commented Jul 9, 2019

If you change the definition from enum to enum class, does it fix your compile error? The additional scope is a bit annoying, and I suspect a compiler bug, but we should work around it.

@scudette
Copy link
Collaborator

scudette commented Jul 9, 2019

Hi Joe,
This is a plain Ubuntu 18.10 system:

$ g++ --version
g++ (Ubuntu 8.3.0-6ubuntu1~18.10.1) 8.3.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Changing to enum class fails with other errors:

In file included from ../aff4/data_store.h:34,
                 from ../aff4/aff4_io.h:29,
                 from ../aff4/libaff4.h:23,
                 from aff4imager.cc:20:
../aff4/attributes.h:99:54: error: no match for ‘operator==’ (operand types are ‘const long unsigned int’ and ‘aff4::AttributeValue::type’)
     static_assert(absl::variant_size<storage>::value == type::_COUNT_,
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
../aff4/attributes.h: In member function ‘constexpr bool aff4::AttributeValue::IsValid() const’:
../aff4/attributes.h:157:25: error: ‘INVALID’ was not declared in this scope
         return !(IsType(INVALID) || val.valueless_by_exception());
                         ^~~~~~~
../aff4/attributes.h:157:25: note: suggested alternative: ‘EINVAL’
         return !(IsType(INVALID) || val.valueless_by_exception());
                         ^~~~~~~
                         EINVAL

I installed abseil using the following steps:

$ git clone https://github.com/abseil/abseil-cpp.git
$ cd abseil-cpp
$ cmake -DCMAKE_INSTALL_PREFIX:PATH=$PREFIX . && make all install

I know there is a lot of flux in the C++ world these days, do we want to remain mostly backwards compatible or do we move our minimum requirements to c++14 ? I dont see any problem with moving on as long as we have a working docker build environment.

@jtsylve
Copy link
Contributor Author

jtsylve commented Jul 9, 2019

Interesting. I think it's a gcc bug. Here's a minimal example: https://godbolt.org/z/YdkLxW

This can be fixed by specifying the namespace: https://godbolt.org/z/dLm3ct

I'm updating the PR to support this. I think we can stay with C++11 for now until we have a real need to raise the minimum requirements.

@jtsylve
Copy link
Contributor Author

jtsylve commented Jul 9, 2019

PR updated. Give it a try

@jtsylve
Copy link
Contributor Author

jtsylve commented Aug 19, 2019

Merge conflicts resolved. @scudette have you gotten a chance to look at this since I fixed the issues?

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