[PATCH] D77133: [mlir] Add an out-of-tree dialect example

[Marius Brehler via Phabricator via llvm-commits]

marbre accepted this revision.
marbre marked an inline comment as done.
marbre added inline comments.


================
Comment at: mlir/examples/standalone/standalone-opt/standalone-opt.cpp:25
+using namespace llvm;
+using namespace mlir;
+
----------------
lattner wrote:
> Kayjukh wrote:
> > lattner wrote:
> > > Kayjukh wrote:
> > > > marbre wrote:
> > > > > Kayjukh wrote:
> > > > > > marbre wrote:
> > > > > > > If it is an out-of-tree example, I would avoid `using namespace`.
> > > > > > Is there any particular reason for this? IMO the two `using namespace` directives make the code easier to read by removing a lot of syntactic overhead. I can understand that in a larger project there might be a potential issue with names colliding but if this is the case, then the maintainer of said project can remove those lines and adapt the code as needed.
> > > > > It might not be necessary here, but I thought it might be nice to follow the [[ https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf6-use-using-namespace-directives-for-transition-for-foundation-libraries-such-as-std-or-within-a-local-scope-only | C++ Core Guidelines ]]. The [[ https://google.github.io/styleguide/cppguide.html#Namespaces | Google C++ Style Guide ]] gives a similar advice: "Do not use using-directives (e.g. using namespace foo)."
> > > > > Further, Intel did avoid copying `using namespace` in their [[ https://github.com/NervanaSystems/ngraph/blob/master/src/contrib/mlir/tools/ngraph-opt/ngraph_opt.cpp | ngraph-opt  ]] and I didin the forked source for `iree-opt`.
> > > > Ok, yes, it makes sense to follow the style guides of other out-of-tree projects.
> > > In my work on an out of tree projects using MLIR, I found it to be pretty impractical to avoid using declarations in general.  There is significant syntactic overhead for this, and (IIRC) tblgen output assumes MLIR is in scope in some cases.  Similarly, LLVM datatypes like StringRef are pervasive and used by downstream projects.
> > > 
> > > I have always ended up using a project specific version of things like this file:
> > > https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Support/LLVM.h
> > > 
> > > but which includes MLIR specific types in addition to LLVM.
> > > 
> > > 
> > I think this mostly comes down to personal preferences and coding habits. Personally, I'm not really a fan of enforcing the "no using declarations" rule for every single piece of code, but I can understand that this is a rather controversial opinion in some places.
> > 
> > Maybe we should aim for a middle ground for this patch? The using declarations are still present in dialect-related source files (`StandaloneDialect.cpp` in this case), but not in the `standalone-opt` tool source code. This way we don't mess with TableGen regarding name scopes but still avoid `using` in the main executable.
> Yes, I agree.  I don't think that perfection is required here, this is an example not a requirement for using MLIR.  I'd optimize towards something that lets the code **look nice** in the example.  
> 
> People who have local standards that frown upon "using namespace" can and will enforce their local conventions if they base anything on the example.
> 
> -Chris
Actually, it was just a suggestion, which I thought would be worth to mention, and nothing I tried to enforce :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77133/new/

https://reviews.llvm.org/D77133