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

[Marius Brehler via Phabricator via llvm-commits]

marbre added inline comments.


================
Comment at: mlir/examples/standalone/standalone-opt/standalone-opt.cpp:25
+using namespace llvm;
+using namespace mlir;
+
----------------
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`.


================
Comment at: mlir/examples/standalone/standalone-opt/standalone-opt.cpp:61
+  registerDialect<standalone::StandaloneDialect>();
+  // TODO: Register standalone passes here.
+
----------------
Kayjukh wrote:
> marbre wrote:
> > A standalone pass is out of scope of this example?
> I was thinking about adding one in a potential follow-up to this patch. As mentioned in the description, this is a bare bones example that could largely be expanded upon. But I think it makes more sense to add additional features one at a time so as not to make a huge patch with everything one would think of in one go. Would this approach make more sense?
Yes, I am fine with that :)


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

https://reviews.llvm.org/D77133