Filter functions, extern vars, defines and unused types from included headers #113

kornilova203 · 2018-07-07T08:13:33Z

Closes #13

Ignore functions, extern variables and defines from included headers.

Removed HeaderManager, added LocationManager that creates instances of Location for each TypeDef.

Location has 2 subclasses:

ScalaLocation means that type is defined in another scala object (not supported yet)
SourceLocation indicates that type is defined in main header or in included header

If type is defined in included header and it is not used then it is removed.

Now I think that ScalaLocation is a bad idea because it does not prevent creation of Struct, Union etc and TypeDef instances.
So it would be better to create an ImportedType instead of TypeDef with ScalaLocation.

TODO:

Add tests

jonas

Made a first pass. It looks like it is possible to simplify a lot of stuff and even get rid of LocationManager in favour of having more details in each Location instance and moving filtering to the generator (llvm::raw_ostream &operator<<(llvm::raw_ostream &s, const IR &ir)) where I feel it belongs.

jonas · 2018-07-07T22:49:19Z

bindgen/visitor/TreeVisitor.cpp

@@ -69,8 +71,8 @@ bool TreeVisitor::VisitEnumDecl(clang::EnumDecl *enumdecl) {
    std::string scalaType = typeTranslator.getTypeFromTypeMap(
        enumdecl->getIntegerType().getUnqualifiedType().getAsString());

-    std::shared_ptr<Type> alias =
-        ir.addEnum(name, scalaType, std::move(enumerators));
+    std::shared_ptr<Type> alias = ir.addEnum(


Unused assignment

jonas · 2018-07-07T22:54:11Z

bindgen/visitor/TreeVisitor.cpp

@@ -1,10 +1,12 @@
 #include "TreeVisitor.h"

-HeaderManager headerMan;
-
 std::set<std::string> locations;


It looks like this is no longer used and can be removed.

jonas · 2018-07-07T23:00:38Z

bindgen/ir/Enum.cpp

    assert(!isAnonymous());
-    return std::make_shared<TypeDef>("enum_" + name, shared_from_this());
+    return std::make_shared<TypeDef>("enum_" + name, shared_from_this(),
+                                     std::move(location));


I'm not sure I understand why the enum doesn't have a location itself, which can be "reused" here.

Also, from TypeDef::location doc string:

nullptr if type is located in main file or is generated.

this is generated, no?

sorry, docstring is wrong, location is null only if it is generated

jonas · 2018-07-07T23:04:29Z

bindgen/ir/IR.cpp

        return typeDefs.back();
    }
    return nullptr;
 }

 void IR::addStruct(std::string name, std::vector<Field *> fields,
-                   uint64_t typeSize) {
+                   uint64_t typeSize, std::shared_ptr<Location> location) {
    std::shared_ptr<Struct> s =
        std::make_shared<Struct>(name, std::move(fields), typeSize);


Again, I'm curious why the location is associated with the generated typedef instead of the "concrete" type.
The latter would require fewer changes, e.g. no need for TypeDef::setLocation().

TypeDef instance in any case should have location because it may reference any type: typedef int size.

Struct, Union and Enum are not referenced directly, corresponding TypeDef is generated for each instance, so it is okay to use location from TypeDef.

It is possible to add location to them too, but in some sense TypeDef replaces actual type, and we store Struct, .. only for helper methods.

So when generator checks for unused external types it can go through all typedefs and check their location.
If a TypeDef is removed and it references a struct, the struct is also removed.

I am not sure if it was right choice, I can try to add location to Struct etc and see if it will make code more complicated.

jonas · 2018-07-07T23:11:01Z

bindgen/visitor/ScalaFrontendAction.h

@@ -12,14 +12,15 @@
 */
 class ScalaFrontendAction : public clang::ASTFrontendAction {
  public:
-    explicit ScalaFrontendAction(IR &ir);
+    ScalaFrontendAction(IR &ir, LocationManager &locationManager);


LocationManager is declared in ir/location so it makes sense to add it as a member to IR to avoid passing it where IR is passed, etc. Better yet might be to have Location under ir/ but have the LocationManager be part of the TreeVisitor so it has access to the SourceManager. We don't need the manager once we are done with the AST because we have the locations themselves.

jonas · 2018-07-07T23:14:54Z

bindgen/ir/location/LocationManager.cpp

+    : mainHeaderPath(std::move(mainHeaderPath)) {}
+
+void LocationManager::loadConfig(const std::string &path) {
+    // TODO: implement


Will this use the stdHeader.config file?

jonas · 2018-07-07T23:23:47Z

bindgen/defines/DefineFinder.cpp

@@ -100,6 +104,9 @@ void DefineFinder::MacroUndefined(const clang::Token &macroNameTok,
                                  const clang::MacroDefinition &md,
                                  const clang::MacroDirective *undef) {
    clang::SourceManager &sm = compiler.getSourceManager();
+    if (!sm.isInMainFile(undef->getLocation())) {
+        return;
+    }


Having a test for this would be very valuable.

jonas · 2018-07-07T23:28:10Z

bindgen/ir/location/Location.h

+class Location {
+  public:
+    virtual ~Location() = default;
+};


I think it would be better to have a single Location class with the include file path, isInMainFile and position in the file for error reporting etc. It sounds like you are considering that. If we put the position there it would also render much (or all) of the LocationManager obsolete because the TreeVisitor will just generate a new Location every time. The mapping from file path to Scala module path can then be done in the generator.

jonas · 2018-07-07T23:28:47Z

bindgen/ir/location/LocationManager.cpp

+
+std::shared_ptr<Location>
+LocationManager::getLocation(std::string pathToHeader,
+                             std::string includeLocation) {


What's the purpose of includeLocation?

pathToHeader minus includeLocation will give us relative path to header (the path that is used in include for example bits/stdio.h).
stdHeader file should contain these relative paths (not only header names) so we will be able to correctly map headers to existing bindings.

Oh no includeLocation is not an include path

jonas · 2018-07-07T23:37:58Z

bindgen/ir/IR.cpp

@@ -353,3 +365,47 @@ IR::~IR() {
    variables.clear();
    varDefines.clear();
 }
+
+void IR::removeUnusedExternalTypes() {


I don't think it makes sense to remove types, since we loose a lot of information. We need the external types to correctly reference types from other modules, like stdio.FILE. It would be better to update the generators to skip when !isInMainFile and then have them also prefix the module path when referencing an external type.

kornilova203 · 2018-07-09T18:21:52Z

Not ready for review

jonas · 2018-07-10T10:25:32Z

bindgen/Main.cpp

-    }
-
-    locations.clear();
+    assert(op.getSourcePathList().size() == 1);


Is it possible to lift this requirement? If it is not possible at this point, it would be good to exit with a proper error message.

I will lift the requirement and add proper error message

jonas · 2018-07-10T10:30:11Z

bindgen/TypeTranslator.cpp

@@ -102,7 +102,7 @@ TypeTranslator::translateStructOrUnionOrEnum(const clang::QualType &qtpe) {
    /* type is not yet defined.
     * TypeDef with nullptr will be created.
     * nullptr will be replaced by actual type when the type is declared. */
-    typeDef = ir.addTypeDef(nameWithoutSpace, nullptr);
+    typeDef = ir.addTypeDef(nameWithoutSpace, nullptr, nullptr);


Eventually passing a location here would allow us to provide a better diagnostic if an opaque type is used in a problematic way.

It is a good idea but we should not use location field for this.
Maybe create an optional field typeDeclarationLocation

I started doing it but it required passing Location instance to methods of TypeTranslator and it does not look good to me, so I decided to leave it for now.
I hope that after fixing #106 we will not see the error message often (except the case when one wants to generated bindings for broken header).

jonas · 2018-07-10T10:46:58Z

bindgen/ir/IR.cpp

@@ -171,18 +179,19 @@ void IR::generate(const std::string &excludePrefix) {
    if (!generated) {
        setScalaNames();
        filterDeclarations(excludePrefix);
+        //        removeUnusedExternalTypedefs();


Remove commented out code.

jonas · 2018-07-10T10:55:07Z

bindgen/ir/IR.cpp

+}
+
+template <typename T>
+bool IR::isOutputted(const std::shared_ptr<T> &type) const {


I found the name a bit confusing, maybe shouldGenerate or shouldOutput?

jonas · 2018-07-10T11:01:34Z

bindgen/ir/Struct.cpp

@@ -118,7 +128,7 @@ bool Struct::hasHelperMethods() const {
    return !fields.empty() && fields.size() < SCALA_NATIVE_MAX_STRUCT_FIELDS;
 }

-std::string Struct::getAliasType() const { return "struct_" + name; }
+std::string Struct::getTypeAlias() const { return "struct_" + name; }


jonas · 2018-07-10T11:03:42Z

bindgen/ir/Struct.cpp

@@ -176,3 +196,11 @@ std::string Union::generateHelperClass() const {
 }

 std::string Union::getTypeAlias() const { return "union_" + name; }
+
+bool Union::operator==(const Type &other) const {
+    auto *structOrUnion = dynamic_cast<const StructOrUnion *>(&other);


Why not cast to Union?

Cast type to Union/Struct in operator== of Union/Struct class Fix error message when definition of opaque type was not found

kornilova203 added 3 commits July 7, 2018 10:54

Ignore functions, defines and variables from included headers

2f9f9bf

Remove unused external types

919c4be

Remove HeaderManager and locations

a0a616b

kornilova203 changed the title ~~[WIP] Only declarations from original header~~ [WIP] Filter functions, extern vars, defines and unused types from included headers Jul 7, 2018

jonas reviewed Jul 7, 2018

View reviewed changes

kornilova203 added 3 commits July 9, 2018 06:57

Remove unused code

331dabe

Add location to Struct, Union and Enum

bce760d

Remove subclasses of Location. Don't remove not printed declarations

902fa75

Add tests. Search for type usages recursively

5447079

kornilova203 force-pushed the only-declarations-from-original-header branch from f2e29e6 to 5447079 Compare July 10, 2018 08:15

jonas approved these changes Jul 10, 2018

View reviewed changes

kornilova203 force-pushed the only-declarations-from-original-header branch 2 times, most recently from 4cdb3d5 to cb62f50 Compare July 10, 2018 17:10

kornilova203 changed the title ~~[WIP] Filter functions, extern vars, defines and unused types from included headers~~ Filter functions, extern vars, defines and unused types from included headers Jul 10, 2018

Check number of headers before running bindgen

7e45389

Cast type to Union/Struct in operator== of Union/Struct class Fix error message when definition of opaque type was not found

kornilova203 force-pushed the only-declarations-from-original-header branch from cb62f50 to 7e45389 Compare July 10, 2018 17:46

kornilova203 merged commit 7c61f98 into master Jul 10, 2018

kornilova203 deleted the only-declarations-from-original-header branch July 10, 2018 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter functions, extern vars, defines and unused types from included headers #113

Filter functions, extern vars, defines and unused types from included headers #113

kornilova203 commented Jul 7, 2018 •

edited

Loading

jonas left a comment

jonas Jul 7, 2018

jonas Jul 7, 2018

jonas Jul 7, 2018

kornilova203 Jul 9, 2018

jonas Jul 7, 2018

kornilova203 Jul 9, 2018

jonas Jul 7, 2018

jonas Jul 7, 2018

kornilova203 Jul 9, 2018

jonas Jul 7, 2018

jonas Jul 7, 2018

jonas Jul 7, 2018

kornilova203 Jul 9, 2018

kornilova203 Jul 9, 2018

jonas Jul 7, 2018 •

edited

Loading

kornilova203 commented Jul 9, 2018

jonas Jul 10, 2018

kornilova203 Jul 10, 2018

jonas Jul 10, 2018

kornilova203 Jul 10, 2018

kornilova203 Jul 10, 2018

jonas Jul 10, 2018

jonas Jul 10, 2018

kornilova203 Jul 10, 2018

jonas Jul 10, 2018

jonas Jul 10, 2018

Filter functions, extern vars, defines and unused types from included headers #113

Filter functions, extern vars, defines and unused types from included headers #113

Conversation

kornilova203 commented Jul 7, 2018 • edited Loading

jonas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

kornilova203 commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornilova203 commented Jul 7, 2018 •

edited

Loading

jonas Jul 7, 2018 •

edited

Loading