Issue 6032593782833152: Issue 1173 - proposal for entry point (Closed)
Patch Set 1 #Patch Set 2 : base on current tip #
MessagesTotal messages: 16
I am completely against using this approach _at the present time_. This is premature optimization of the worst kind, because it's not clear that what's proposed is even the correct behavior. We currently have no experience with catching exceptions at all. We don't know what the actual behavior with exception handlers will be. In particular, we don't know whether exception handling that is completely identical everywhere is the right behavior. At the very least, we have no evidence to suggest that it _is_ the right behavior. I have a strong suspicion that it is, in fact, the _wrong_ behavior. Having said all that, using an approach like this _might_ be appropriate to consider _in the future_, _after_ we have identified all the entry points and completed the task of getting try-catch statements around all their function bodies. Once that's done, and we have some idea about the circumstances under which the catch blocks are activated, only then is it time to consider whether we should use this kind of approach.
I like this approach. If we are going to do this (for which I am rather ambivalent, and I still don't think this should be on any high priority at all) I think this would be a way to go. @Eric, what would be a scenario where this would not be enough?
On 2015/02/02 13:25:22, Eric wrote:
> I am completely against using this approach _at the present time_. This is
> premature optimization of the worst kind, because it's not clear that what's
> proposed is even the correct behavior.
I don't think it's premature optimization because it already prevents us from
code duplicating in many places. Compare copy-pasting `try { ... } catch
(...){...}` and calling this functions. I would also say that it has better
readability than multi-line exception wrappers. Even if we find that it has some
issues, I guess, it will be much easier to fix it for all entries at once as
well as it provides more reliable way to identify all cases where we use it than
going through the commented methods.
>
> We currently have no experience with catching exceptions at all. We don't know
> what the actual behavior with exception handlers will be. In particular, we
> don't know whether exception handling that is completely identical everywhere
is
> the right behavior. At the very least, we have no evidence to suggest that it
> _is_ the right behavior. I have a strong suspicion that it is, in fact, the
> _wrong_ behavior.
How does having try-catch in each method help here? Even if we discover that
only some methods require to work with a different set of exceptions, which I
personally doubt, it seems easier to replace the wrapper functions for those
methods than changing the catching of the exceptions in each method.
BTW, we still can write try{}catch(){} in whatever place, so it does not apply
some limitations neither complicates the support of it.
On 2015/02/04 10:14:26, Oleksandr wrote:
> I like this approach. If we are going to do this (for which I am rather
> ambivalent, and I still don't think this should be on any high priority at
all)
> I think this would be a way to go. @Eric, what would be a scenario where this
> would not be enough?
JIC, we have already a couple of methods wrapped by try-catch, so some further
change can remove it. I would like to have it already to avoid any such
situations.
On 2015/02/04 10:14:26, Oleksandr wrote: > I like this approach. If we are going to do this (for which I am rather > ambivalent, and I still don't think this should be on any high priority at all) > I think this would be a way to go. Well, you've also stated in the past that you don't like see code littered with try-catch statements, so I'm not surprised you'd like to hide them away somewhere. However, if we want to use RAII consistently, we need to be throwing exceptions in constructors and we need try-catch statements to catch those exceptions, and we'll need those try-catch statements in more than just the outer surface of entry point functions. (Not that we need to catch every exception, that's not the point.) > @Eric, what would be a scenario where this > would not be enough? In some circumstances should be disabling the plugin rather than just returning an error code, for example. We have somewhere between one and two dozen entry points in the code. Do all these entry points even use the same error codes to represent failure? COM is notoriously inconsistent in this way. I am entirely unwilling to make the claim that I know that the correct behavior for all of the entry points. I am perfectly willing to say that I expect that the correct behavior is not identical in all cases.
On 2015/02/04 11:31:10, sergei wrote:
> I don't think it's premature optimization because it already prevents us from
> code duplicating in many places.
You're making an assumption that the code will actually be identical in these
places. Can you provide me evidence that it will be the same in all these
places?
As first step towards this goal, can you provide a complete enumeration of all
the entry points we would be using this proposal and pointers to the
documentation that states what the correct error code is?
> Compare copy-pasting `try { ... } catch
> (...){...}` and calling this functions. I would also say that it has better
> readability than multi-line exception wrappers.
try-catch is easier to read because it's the C++ idiom for exception handling,
and this method is not. I do not think this is a legibility win.
> Even if we find that it has some
> issues, I guess, it will be much easier to fix it for all entries at once as
> well as it provides more reliable way to identify all cases where we use it
than
> going through the commented methods.
This is also a spurious argument. A consistent, single-line comment identifying
entry points is just as reliable for finding the entry points. Either use 'grep'
or 'Find All' in Visual Studio.
> BTW, we still can write try{}catch(){} in whatever place, so it does not apply
> some limitations neither complicates the support of it.
When we write try-catch _in addition_ to using a wrapper, we're splitting
exception behavior for a single function between two places. I am dubious that
such a split is an improvement. It might be, since one is default behavior and
the other is not, but I am not prepared at this point to concede this point, and
the reason is that we have no evidence that this is the case.
On 2015/02/04 12:50:35, Eric wrote: > However, if we want to use RAII consistently, we need to be throwing exceptions > in constructors and we need try-catch statements to catch those exceptions, and > we'll need those try-catch statements in more than just the outer surface of > entry point functions. (Not that we need to catch every exception, that's not > the point.) Could you provide the example when we need it? Pay attention that we capture all variables into the closure by reference ([&]), thus there are no calls of copy-constructors neither any constructors. The only one constructor which is called here is the std::function(F f), it's the 5th version of the constructor http://en.cppreference.com/w/cpp/utility/functional/function/function. > > > @Eric, what would be a scenario where this > > would not be enough? > > In some circumstances should be disabling the plugin rather than just returning > an error code, for example. > > We have somewhere between one and two dozen entry points in the code. Do all > these entry points even use the same error codes to represent failure? COM is > notoriously inconsistent in this way. > > I am entirely unwilling to make the claim that I know that the correct behavior > for all of the entry points. I am perfectly willing to say that I expect that > the correct behavior is not identical in all cases.
Sorry, accidentally sent incomplete message.
On 2015/02/04 12:50:35, Eric wrote: > However, if we want to use RAII consistently, we need to be throwing exceptions > in constructors and we need try-catch statements to catch those exceptions, and > we'll need those try-catch statements in more than just the outer surface of > entry point functions. (Not that we need to catch every exception, that's not > the point.) Could you provide the example when we need it? Pay attention that we capture all variables into the closure by reference ([&]), thus there are no calls of copy-constructors neither any constructors. The only one constructor which is called here is the std::function(F f), it's the 5th version of the constructor http://en.cppreference.com/w/cpp/utility/functional/function/function. Yes, it can throw the exception std::bad_alloc. But it's much better to know that it can throw only this one exception. May be we can use #define to wrap only this one exception, but there is really no point to write try catch in each method. Honestly, I guess, such construction with lambda will be inlined with the closure on the stack. > > @Eric, what would be a scenario where this > > would not be enough? > > In some circumstances should be disabling the plugin rather than just returning > an error code, for example. I would say, that if exception caught in any entry point function, these entry point wrappers are the best central place to set some global flag and change the behaviour of all entry points at once. So, it's more flexible than going through all entry points and putting such logic into each method. > You're making an assumption that the code will actually be identical in these > places. Can you provide me evidence that it will be the same in all these > places? > > As first step towards this goal, can you provide a complete enumeration of all > the entry points we would be using this proposal and pointers to the > documentation that states what the correct error code is? > ... > We have somewhere between one and two dozen entry points in the code. Do all > these entry points even use the same error codes to represent failure? COM is > notoriously inconsistent in this way. > > I am entirely unwilling to make the claim that I know that the correct behavior > for all of the entry points. I am perfectly willing to say that I expect that > the correct behavior is not identical in all cases. You are right, we have many different entry points spread over the files but I'm not making such assumption. For each particular case we can create a special wrapper function and it will be easier to see the difference between them and recognize some patterns there when we have all of them in one place (in one file, for example) than working with handlers spread over files. > > Compare copy-pasting `try { ... } catch > > (...){...}` and calling this functions. I would also say that it has better > > readability than multi-line exception wrappers. > > try-catch is easier to read because it's the C++ idiom for exception handling, > and this method is not. I do not think this is a legibility win. It's not easier to read because of additional conditional logic. > > > Even if we find that it has some > > issues, I guess, it will be much easier to fix it for all entries at once as > > well as it provides more reliable way to identify all cases where we use it > than > > going through the commented methods. > > This is also a spurious argument. A consistent, single-line comment identifying > entry points is just as reliable for finding the entry points. Either use 'grep' > or 'Find All' in Visual Studio. What is more reliable using 'Find All' in Visual Studio or 'grep' or compiler error message? Apparently, compiler error message is more reliable. As well as we don't care about keeping of these comments up to date. > > BTW, we still can write try{}catch(){} in whatever place, so it does not apply > > some limitations neither complicates the support of it. > > When we write try-catch _in addition_ to using a wrapper, we're splitting > exception behavior for a single function between two places. I am dubious that > such a split is an improvement. It might be, since one is default behavior and > the other is not, but I am not prepared at this point to concede this point, and > the reason is that we have no evidence that this is the case. It's not in addition, it's instead of using wrapper functions. We can easily get rid of wrapper functions at any point and compiler will tell us if miss something.
The central question here is who has the burden of proof about whether this proposal is a good idea at the present time. Either I have to demonstrate now how the code will need to be different in the future or you all have to demonstrate now how the code will never need to be different in the future. My original point, which no one addressed, is that we do not have evidence to support either side well. My presumption is that the code will be different because code is generically different. I consider that a more conservative claim that the one that says that the code will be the same. Regardless, I have no direct evidence that the code should be different, because we haven't written the code yet. Likewise, you all have no evidence that the code should be the same for the same reason, because we haven't written the code yet. One of the good lessons from the agile development community is that you should (1) first get code working and (2) only then refactor the code. The present proposal is a possibility for step (2). My main point is that we should do step (1) first. This is why I called it premature optimization; it's a way of removing duplication for code that we do not have evidence is actually duplicated, because it's not present yet. We haven't even identified all the entry points yet! The ticket has an initial list of them, but as I said in that comment, I don't know yet that it's complete. And then once we have a list of entry points, we need to put at least some non-zero amount of thought into what the correct behavior should be in exceptional circumstances. None of this has been done yet. My argument about this proposal is not about technical merit. It's about development process.
I can say, that it's good at the present time. Because the trouble is already here. Remember http://codereview.adblockplus.org/4937147073167360/ it splits Invoke into 7 methods, I don't want to have the same try catch 7 times, it can be wrapper functions, defines or what ever, but there is no any reason to duplicate it. In addition there is SetSite. We can continue to identify all entry points using this approach as well as we should continue to dig into it, whether it's good or not, but I'm pretty sure that even if we decide to completely change it in the future it will be much easier to find all cases using compilation errors.
On 2015/02/04 13:30:14, sergei wrote: > Could you provide the example when we need it? Can you provide a proof that we don't need it? Our messages crossed in transit; I addressed this issue in a message just a few minutes after yours. > Pay attention that we capture all > variables into the closure by reference ([&]), thus there are no calls of > copy-constructors neither any constructors. [...] My objections don't have to do with technical merit. I won't address such issues here, because I consider them premature. > I would say, that if exception caught in any entry point function, these entry > point wrappers are the best central place to set some global flag and change the > behaviour of all entry points at once. So, it's more flexible than going through > all entry points and putting such logic into each method. I fully expect that we'll have a function that enacts default behavior that will be called in catch(...) blocks. I'm not arguing against the fact that we will find we have some common behavior. I am arguing that a mechanism that enforces a single behavior everywhere is premature and has insufficient evidence to support it. > You are right, we have many different entry points spread over the files but I'm > not making such assumption. For each particular case we can create a special > wrapper function [...] "Special wrapper function"? That seems just absurd and backwards to me. If the behavior is different, just put the different behavior in a try-catch statement in the same function! > > try-catch is easier to read because it's the C++ idiom for exception handling, > > and this method is not. I do not think this is a legibility win. > It's not easier to read because of additional conditional logic. We disagree, then. > What is more reliable using 'Find All' in Visual Studio or 'grep' or compiler > error message? Apparently, compiler error message is more reliable. As well as > we don't care about keeping of these comments up to date. How are you going to get a compiler error message about a wrapper function that correctly compiles? Perhaps I don't know what you're concerned with, but I see no practical difference in identifying entry points between searching for a comment and searching for a wrapper function. As for keeping up to date, we add new entry points extremely infrequently. There's no particular burden here. If you'd like a compiler-supported mechanism, we could use C++11 attribute syntax. Unfortunately that's not present even in Visual Studio 2013 (and we're not updated to that one yet). Nor is it in the present VS 2015 preview. So I'm afraid that's not much help. http://blogs.msdn.com/b/vcblog/archive/2014/11/17/c-11-14-17-features-in-vs-2... > It's not in addition, it's instead of using wrapper functions. We can easily get > rid of wrapper functions at any point and compiler will tell us if miss > something. What compiler message tells us if we miss anything? The dynamic exception specification in C++98 is understood to be broken, since it doesn't prevent a function from throwing an undeclared exception. (That why C++11 'noexcept' uses the dual concept, because it's enforceable.)
On 2015/02/04 13:47:21, sergei wrote:
> even if we decide to completely change it in the future it will be much
> easier to find all cases using compilation errors.
If you want to find entry points with compiler errors by undefining a symbol,
simply declare the following function in some header and call it as the first
line of the function body of each entry point.
void EntryPoint(){};
Comment out the declaration to generate errors.
I have no problem with this.
I like the concept, but:
1. I agree that try [] catch (...) {} is easier on the eyes.
2. I don't think currently there's any need for this generalization. For
http://codereview.adblockplus.org/4937147073167360/ the win of having these
EntryPoint functions is minimal, if any. As I see it this only eliminates the
code duplication of standard try-catch blocks by a different duplication of
elaborate function call.
3. I don't think we need try-catch blocks in *every* entry point. I really don't
think having that in default QueryInterface implementation will do anything else
then decrease readability. So maybe in the end we will decide not to introduce
this generalization at all.
So currently I agree with Eric. Let's just stick to try-catch blocks for now.
Who knows how we will decide to tackle this in the end. Maybe we'll decide to
use something like https://msdn.microsoft.com/en-us/library/ms680634.aspx after
all.
On 2015/02/24 08:40:56, Oleksandr wrote:
> I like the concept, but:
>
> 1. I agree that try [] catch (...) {} is easier on the eyes.
> 2. I don't think currently there's any need for this generalization. For
> http://codereview.adblockplus.org/4937147073167360/ the win of having these
> EntryPoint functions is minimal, if any. As I see it this only eliminates the
> code duplication of standard try-catch blocks by a different duplication of
> elaborate function call.
> 3. I don't think we need try-catch blocks in *every* entry point. I really
don't
> think having that in default QueryInterface implementation will do anything
else
> then decrease readability. So maybe in the end we will decide not to introduce
> this generalization at all.
>
> So currently I agree with Eric. Let's just stick to try-catch blocks for now.
OK, thanks.
> Who knows how we will decide to tackle this in the end. Maybe we'll decide to
> use something like https://msdn.microsoft.com/en-us/library/ms680634.aspx
after
> all.
It's not recommended,
https://msdn.microsoft.com/en-us/library/aa753617(v=vs.85).aspx#wsbe_Exceptio...
On 2015/02/24 08:40:56, Oleksandr wrote: > 3. I don't think we need try-catch blocks in *every* entry point. I really don't > think having that in default QueryInterface implementation will do anything else > then decrease readability. Indeed the new implementation of CPluginUserSettings::QueryInterface does not use try-catch because there's no statement that can throw within it. We don't have 'noexcept' in our currently compiler, so instead there's a comment on the one statement that looks like it _might_ throw. The result we want is to eliminate exceptions from propagating out of entry points. 'try'-'catch' does this, as does 'noexcept'. (Aside: 'noexcept' is not in VS 2013, but it is in VS 2015 Preview. See http://blogs.msdn.com/b/vcblog/archive/2014/11/17/c-11-14-17-features-in-vs-2...) > Maybe we'll decide to > use something like https://msdn.microsoft.com/en-us/library/ms680634.aspx after > all. Sergei's right; unhandled-exception handlers are fraught with peril. Their best use is to terminate a program as gracefully as possible, given possible program corruption. In our case that would mean having a something like a global disable for everything, including all UI. They can also be usefully used to terminate individual threads, if you've built the infrastructure for that. It's certainly nothing we're ready for yet. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
