This is an archive of the discontinued LLVM Phabricator instance.

Move several plugin to its own namespace
ClosedPublic

Authored by tberghammer on Mar 27 2015, 3:28 AM.

Details

Summary

Move several plugin to its own namespace

Affected paths:

  • Plugins/Platform/Android/*
  • Plugins/Platform/Linux/*
  • Plugins/Platform/gdb-server/*
  • Plugins/Process/Linux/*
  • Plugins/Process/gdb-remote/*

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer retitled this revision from to Move lot of class from the global namespace into lldb_private.
tberghammer updated this object.
tberghammer edited the test plan for this revision. (Show Details)
tberghammer added reviewers: ovyalov, clayborg.
tberghammer added a subscriber: Unknown Object (MLST).
clayborg edited edge metadata.Mar 27 2015, 9:55 AM

The main question is do we want to move each plug-in into its own namespace within lldb_private namespace?

All code from the GDB remote process plug-in could be in:

namespace lldb_private {
  namespace plugin {
    namespace process {
      namespace gdb_remote {
        // Code here
      }
    }
  }
}

Or just

namespace lldb_private {
  namespace process_gdb_remote {
        // Code here
  }
}
ovyalov accepted this revision.Mar 27 2015, 9:55 AM
ovyalov edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Mar 27 2015, 9:55 AM

The main question is do we want to move each plug-in into its own namespace within lldb_private namespace?

All code from the GDB remote process plug-in could be in:

namespace lldb_private {
  namespace plugin {
    namespace process {
      namespace gdb_remote {
        // Code here
      }
    }
  }
}

Or just

namespace lldb_private {
  namespace process_gdb_remote {
        // Code here
  }
}

I am not sure if moving each plugin into its own namespace will make too much sense because most of the plugin is very small (1 or 2 cpp file) so the additional namespaces just adding some noise to the code without any real benefit. What I would consider is to leave the public part of the plugins directly in the lldb_private namespace and if a plugin require additional classes which are private to the plugin then put them into a nested namespace inside lldb_private.

From the two namespace structure you suggested I would prefer the second one as the first one create a very deep nesting so if we have to refer to a plugin from a header file outside of that plugin (e.g.: from a platform plugin to a process plugin) then it will create extremely long fully qualified names.

If we want to go with adding nested namespace then I would also consider the following structures because the plugins for the same OS usually tied together in some level:

namespace lldb_private {
  namespace linux {
    namespace process {
      // Code here
    }
  }
}

Or just an OS level separation (then we don't have to postfix almost all class with the OS name as ProcessLinux, PlatformWindows, ...)

namespace lldb_private {
  namespace linux {
    // Code here
  }
}

I would prefer each plug-in use its own namespace, even if the the shallow version is fine. I would just like people to see the error of their ways if they are in ProcessLinux.cpp and you see:

lldb_private::gdb_remote::something();

or if "using namespace lldb_private;":

gdb_remote::something();

This should be a good indication that the abstraction was broken since you are in ProcessLinux.cpp and you are accessing code from a plug-in. Where as if you saw:

lldb_private::something();

or if "using namespace lldb_private;":

something();

You might think the above code would be OK since it is in the lldb_private namespace, whereas seeing "gdb_remote" namespace quickly tells you something is up.

tberghammer retitled this revision from Move lot of class from the global namespace into lldb_private to Move several plugin to its own namespace.
tberghammer updated this object.
tberghammer edited edge metadata.

Move plugins to their own namespace

This change exposed several flows in the current design (mostly classes leaving in the wrong place). I will try to fix them in the future when I have some time for it.

clayborg accepted this revision.Mar 30 2015, 9:53 AM
clayborg edited edge metadata.

lgtm

This revision was automatically updated to reflect the committed changes.