Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small fixes (ref-doc, removed vs2010 warning) #127

Merged
merged 11 commits into from
Aug 21, 2013
Merged

Small fixes (ref-doc, removed vs2010 warning) #127

merged 11 commits into from
Aug 21, 2013

Conversation

bmunkholm
Copy link
Contributor

mode_ReadWriteNoCreate
// Don't change values below.

/// Open in read-only mode. Fail if the file does not already exist.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these changes doing here? We already have an ongoing discussion about them in a different pull-request. Again, I find that you are giving priority to the wrong thing her. It is more important to keep this enum decoupled from the language bindings than to gain the infinitesimal speedup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The language bindings and the C++ Core is completely tightly coupled in numerous ways anyway. If we ever have a need of changing the OpenMode enum, we would anyway have to checkup on all the bindings. There is no gain by a decoupling now. IF we ever need to decouple it, we can easily do that if we want to. Talking about priority, this is such a small issue that I don't think we should give it priority to spend more time discussing it at all :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The language bindings and the C++ Core is completely tightly coupled in
numerous ways anyway.

They are, in the places they they must be.
They mostly aren't in the places where they shouldn't be.
I'm working hard to eliminate all the design gotchas and quirks from the
core library. That is why I get a little itchy when I see that more is
coming in.

If we ever have a need of changing the OpenMode enum, we would anyway have
to checkup on all the bindings.

We would, but with decoupled enums, the fixes will be trivial. Not so with
delicate dependencies that the compiler knows nothing about.
Fixing the language binding in the decouples case is easy, because the
compiler will tell us exactly where to go and fix. Assuming, of course,
that the switch statements have been written without a 'default' case.

There is no gain by a decoupling now.

Yes there is. The road to a clean and safe API is long enough already. If
we don't exercise vigilance now, we risk loosing the opportunity of ever
getting there.

IF we ever need to decouple it, we can easily do that if we want to.

While that is true, the risk that we will choose not to, is too great - for
the reasons I just mentioned.

Talking about priority, this is such a small issue that I don't think we
should give it priority to spend more time discussing it at all :-)

Many small issues build up...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with most - but one valid point is that if we introduce a switch (without default), we will catch when another enum is added - that has value!

@kspangsege
Copy link
Contributor

Except for the hard-wired enum values, 👍

@kspangsege
Copy link
Contributor

👍

bmunkholm pushed a commit that referenced this pull request Aug 21, 2013
Small fixes (ref-doc, removed vs2010 warning)
@bmunkholm bmunkholm merged commit fc2ef54 into master Aug 21, 2013
@bmunkholm bmunkholm deleted the small-fixes branch August 21, 2013 10:41
tgoyne pushed a commit that referenced this pull request Jul 11, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants