Move using-declarations into class body to avoid polluting the global namespace

[kikitte]

Polygon, LineString, etc., are declared in the global namespace with the using-declaration keyword, which is very likely to confilict with other types of the same name.

Move them to geos::operation::buffer fixes this.

geos/include/geos/operation/buffer/OffsetCurve.h

Lines 39 to 50 in 46972ad

	using geos::geom::Coordinate;
	using geos::geom::CoordinateSequence;
	using geos::geom::Geometry;
	using geos::geom::GeometryFactory;
	using geos::geom::LineString;
	using geos::geom::Polygon;
	namespace geos {
	namespace operation {
	namespace buffer {

[dbaston]

Should we instead remove the using declarations from the header file altogether?

[kikitte]

Should we instead remove the using declarations from the header file altogether?

Sure, looks like geos prefer explicit namespace in header file but using namespace in souce file.

[kikitte]

Hi , I thkink I known what you mean, and I notice that a lot of code using such using style which would make types appear in global namespace.

Since c++ api is one way of using geos, such effort to avoid multiple declaration is necessary and meaningful.

The key is using which code style to sove this, remove using then using explicit namespace in header file, or any other?

[pramsey]

The practice in the code base is a mishmash, I can see trying to move to fewer uses of using in headers as a general practice. How about you do that for this PR, and we'll make it a long term ambition.

[dr-jts]

Should we instead remove the using declarations from the header file altogether?

Won't that mean a lot more scoping of names used in multiple places in the file?

Isn't using intended to cut down on this? It makes sense to limit scope to namespaces, but inside namespaces it seems like a good practice for code brevity.

[rouault]

what about doing the following (generalized to similar cases), to both reduce verbosity and amount of lines changed ?

Suggested change

	const geom::Geometry& inputGeom;
	using Geometry = geom::Geometry;
	const Geometry& inputGeom;

[dr-jts]

So use using as a type alias instead of a using declaration? Is this safer in some way?

What is the scope of the alias? Is it just the namespace within the cpp file including the header? Or is it the entire namespace (seems unlikely, since how would other compilation units see it?)

[rouault]

What is the scope of the alias?

the class in which you put it

Not saying this is a good idea. Mostly for brainstorming

[pramsey]

Why not just moving the using geos::geom::Coordinate inside the name space that contains the class definitions in the header, as the original revision of the PR suggested? Seems like we avoid polluting the global name space and can have as much terseness as we feel like, right?

[dbaston]

The practice in the code base is a mishmash, I can see trying to move to fewer uses of using in headers as a general practice.

If you check grep -R "^using.*;" include from the project root, it looks like the use of using in headers is a pretty recent addition.

Isn't using intended to cut down on this? It makes sense to limit scope to namespaces, but inside namespaces it seems like a good practice for code brevity.

I think it's a good practice inside implementation files, where you're not exporting the shorthand to anyone including the header.

Why not just moving the using geos::geom::Coordinate inside the name space that contains the class definitions in the header, as the original revision of the PR suggested? Seems like we avoid polluting the global name space and can have as much terseness as we feel like, right?

Because then we've polluted the geos::operation::buffer namespace, so anyone including this header now has a type called geos::operation::buffer::Coordinate. Is a geos::operation::buffer::Coordinate the same as a geos::geom::Coordinate? You'd need to check the source to see.

If you instead put the using declaration inside the class, as Even showed, you can make it private and avoid exporting it to everybody.

[dr-jts]

If you instead put the using declaration inside the class, as Even showed, you can make it private and avoid exporting it to everybody.

Do you mean "put the using type alias inside the class"? (I.e. the using Name = other:Name syntax)

If so, does the following capture the recommended practice?:

• In header files, names can be shortened by adding type aliases such as using Geometry = geom::Geometry; inside the class definition
• In implementation cpp files, names can be shortened by adding using declarations such as using geom::Geometry;

[dbaston]

Do you mean "put the using type alias inside the class"? (I.e. the using Name = other:Name syntax)

Yes

If so, does the following capture the recommended practice?:
In header files, names can be shortened by adding type aliases such as using Geometry = geom::Geometry; inside the class definition

I don't know if I'd call it a recommend practice, just trying to clarify the difference between putting it inside and outside the class definition. If you look at a header like OffsetCurve.h I'm not sure it's worth it over just writing geom::Geometry. Others could reach a different conclusion.

In implementation cpp files, names can be shortened by adding using declarations such as using geom::Geometry;

Yes

[kikitte]

I'am back, I've learned a lot from your conversations.

It looks like "put the using type alias inside the class" is an acceptable solution, I am trying to fix all the header files in this way: type alias always on the top of the class. So alias is private to the class without leaking.

But according to my statistics, there are at least 59 header files need to fix, it may take several days to finish.

class GEOS_DLL OffsetCurve {
  using Coordinate = geos::geom::Coordinate;
  using CoordinateSequence = geos::geom::CoordinateSequence;
  using Geometry = geos::geom::Geometry;
  using GeometryFactory = geos::geom::GeometryFactory;
  using LineString = geos::geom::LineString;
  using Polygon =geos::geom::Polygon;

private:

    // Members
    const Geometry& inputGeom;
    double distance;
    bool isJoined = false;

[dr-jts]

Are the forward declarations still needed? Would be nice if that cruft could be eliminated.

I guess they need to be replaced by includes. Still fewer lines of code, though, and less fiddly.

// Forward declarations
namespace geos {
namespace geom {
class Coordinate;
class CoordinateSequence;
class Geometry;
class LineString;
class Polygon;
}
namespace operation {
namespace buffer {
class OffsetCurveSection;
class SegmentMCIndex;
}
}
}

[pramsey]

I think there's a compile time penalty to including everything.

[dbaston]

Forward declarations break the dependency chain, so a change in LineString.h does not force a recompilation of everything that includes OffsetCurve.h.

[dr-jts]

Forward declarations break the dependency chain, so a change in LineString.h does not force a recompilation of everything that includes OffsetCurve.h.

Yeesh, how fiddly. But this usage should be encouraged, sounds like.

[kikitte]

Ready for review.

Note that sometimes I prefer using namespace geos::geom in implementation file, I think it is acceptable and pretty common in geos source code.