This is an archive of the discontinued LLVM Phabricator instance.

[flang] Remove 'using namespace mlir;` from header files
ClosedPublic

Authored by awarzynski on Mar 3 2022, 5:38 AM.

Details

Summary

Currently, CGOps.h and FIROps.h contain using namespace mlir;. Every
file that includes one of these header files (directly and transitively)
will have the MLIR namespace enabled. With name-clashes within
sub-projects (LLVM and MLIR, MLIR and Flang), this is not desired. Also,
it is not possible to "un-use" a namespace once it is "used". Instead,
we should try to limit using namespace to implementation files (i.e.
*.cpp).

This patch removes using namespace mlir; from header files and adjusts
other files accordingly. In header and TableGen files, extra namespace
qualifier is added when referring to symbols defined in MLIR. Similar
approach was used for source files that didn't require many changes.
Otherwise, using namespace mlir; is added.

Diff Detail

Event Timeline

awarzynski created this revision.Mar 3 2022, 5:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Mar 3 2022, 5:38 AM
clementval added inline comments.Mar 3 2022, 1:56 PM
flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
20

it's probably mlir::dyn_cast here.

27–29

mlir::isa

flang/lib/Lower/Bridge.cpp
44

Wouldn't it make more sense to just get rid of it since we want fully qualified names? Same for other places.

bondhugula accepted this revision.Mar 3 2022, 4:50 PM
bondhugula added a subscriber: bondhugula.

This is a welcome change in the right direction!

flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
27–29

isa, dyn_cast are re-exported into the MLIR namespace. Given how commonly they are used, you may want to just use them import them into your namespace and use them without the namespace qualifier, or have a using mlir::isa above.

This revision is now accepted and ready to land.Mar 3 2022, 4:50 PM
awarzynski added inline comments.Mar 4 2022, 2:26 AM
flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
20
flang/lib/Lower/Bridge.cpp
44

That would be a much more intrusive patch. I wanted to avoid that while the upstreaming is in progress.

clementval added inline comments.Mar 4 2022, 3:30 AM
flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
20

I know but in the code base we tend to use mlir:: so it would be nice to have homogeneity here as well.

clementval added inline comments.Mar 4 2022, 6:54 AM
flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
20

Just double checked in the flang codebase:
mlir::dyn_cast 93
llvm::dyn_cast 0

awarzynski added inline comments.Mar 4 2022, 11:55 AM
flang/include/flang/Optimizer/Dialect/FIROpsSupport.h
20

I know but in the code base we tend to use mlir:: so it would be nice to have homogeneity here as well.

Not a fan, but that's a very good reason to use mlir::dyn_cast here 👍🏻

Just double checked in the flang codebase:
mlir::dyn_cast 93
llvm::dyn_cast 0

I also found 45 instances of dyn_cast (i.e. without namespace qualifiers).

Replace llvm::<symbol> with mlir::<symbol>

clementval accepted this revision.Mar 9 2022, 12:26 AM

LGTM

flang/lib/Lower/Bridge.cpp
44

Ok makes sense and thanks to be thoughtful about the upstreaming.