Hi Christian,
Unfortunately both options are still not ideal :)
To make all generated Java methods to throw exceptions may require a
lot of changes in existing PJSIP Java apps, which is of course
undesirable.
We still prefer the idea of replacing throw() with PJ_THROW(), which
will translate to no-op for C/C++ and throw() for SWIG. This way,
backward incompatibility is minimized while full feature is
maintained.
Best regards,
Ming
On Wed, Oct 17, 2018 at 4:46 AM, Christian Hoff <christian_hoff@xxxxxxx> wrote:
> I am fordwarding this mail to the mailing list as well.
>
>
> On 16/10/18 22:42, Christian Hoff wrote:
>
> Hello Ming,
>
> you are absolutely right and I am sorry for the confusion. Of course the
> SWIG Java bindings throw "java.lang.Exception" and not the
> "RuntimeException" as I assumed.
>
> You actually discovered a bug in my patch. The problem with my patch is that
> I am throwing a java.lang.Exception from the Java JNI code, but this
> Exception is now no longer mentioned in the "throws" specification of the
> Java methods generated by SWIG - even though it is a checked exception. In
> fact my change (unintentionally) removed the "throw" specification also on
> the Java side! For this reason you no longer get compile errors when
> omitting the "catch" blocks in your sample code. This is a bug. Because the
> Exception is thrown from JNI C++ code, Java cannot find out that the
> generated C++ JNI code is actually violating the Java exception
> specification of the generated method (which declares that no checked
> exceptions should be thrown).
>
> After some reading of the SWIG documentation, I found out that there is a
> relatively simple way to fix this issue and to restore the "throw"
> specification on the Java side. To do this, please replace the line no. 50
> in file "pjsip-apps/src/swig/pj_error.i" by
>
> %javaexception("java.lang.Exception") {
>
> This should restore the "throw" specification for all PJSIP Java methods. I
> tested this briefly and it works. One possible downside of this change is
> however that this change will add a "throw Exception" specification to all
> generated Java methods inside PJSIP, so every PJSIP Java generated method
> now explicitly throws java.lang.Exception. This could be an issue if there
> are some PJSIP methods that cannot throw "pj::Error". In this case the user
> has to handle an exception that can never occur. However I am not even sure
> if such methods exist.
>
> Another option would be throw non-checked exceptions - such as
> RuntimeException - from the PJSIP Java bindings. But I think you like the
> idea of exception specifications and checked exceptions and therefore
> probably the first option will be the better one. As I said, the fix works
> by just changing a single line.
>
>
> Best regards,
>
> Christian
>
>
> On 16/10/18 11:24, Ming wrote:
>
> Hi Christian,
>
> Thanks for your detailed explanation, which I'm sure help us
> (especially me) improve our knowledge about Java.
>
> After checking the code (in pjsua2.i), our current exception actually
> uses java.lang.Exception, which, according to the reference below,
> belongs to checked exceptions. I verified this by trying to compile
> our sample code and remove a try-catch block and it did fail to
> compile.
>
> Ref: https://www.geeksforgeeks.org/checked-vs-unchecked-exceptions-in-java/
>
> Best regards,
> Ming
>
> On Tue, Oct 16, 2018 at 2:45 PM, Christian Hoff <christian_hoff@xxxxxxx>
> wrote:
>
> Hello Ming,
>
> thank you very much for your quick feedback and I am happy to hear that the
> propagation of exceptions works well with my patch!
>
> Regarding your point about exceptions and thr "throw (...)" specification:
> You are right in saying that an exception specification is still an
> important in programming languages such as Java. However in Java two kinds
> of exceptions exist and only one kind of them (the checked exception) works
> with exception specifications:
>
> Checked exceptions. Checked exceptions are the kind of exceptions that you
> are talking about, where the programmer is forced to somehow handle the
> exception. Handling the exception can mean either installing a catch handler
> or adding this exception type to the "throws" declaration of your method. If
> the exception is not handled, a compile error will be generated - as you
> wrote.
> Unchecked exceptions. Unchecked exceptions are the kind of exceptions that
> the programmer is not forced to handle. For this kind of exception, the
> programmer is not forced to install a "catch" handler, so no compile error
> will be generated if the exception is not handled at all. Such exceptions
> also do not appear in the "throws" declaration of the methods. Unchecked
> exceptions are all exceptions that derive from the RuntimeException and
> Error classes.
>
> For PJSIP a "pj::Error" was mapped to a RuntimeException up to now (already
> before my changes), so a "pj::Error" from the C++ PJSIP library raises an
> unchecked exception. This is true before and after my changes. This means
> that - as of now - a Java programmer is not forced to handle a "pj::Error"
> from the C++ code. This is because the "pj::Error" will be mapped to an
> unchecked exception. The consequence is that there is no compile error if
> the user forgets to install a catch handler on the Java side for errors from
> PJSIP. The exception for the "pj::Error" also does not appear in the
> "throws" declaration of the Java methods generated by SWIG as only checked
> exceptions should appear there, but not unchecked exceptions such as
> "RuntimeException".
>
> What I want to say in short is that my patch does not change anything in
> terms of exception handling on the Java side. The old behaviour was already
> to throw an unchecked exception that the user was not required to catch and
> the new approach is the same. And already before my changes we were not
> using exception specifications on the Java side as we were using unchecked
> exceptions. Or have I missed or overlooked something?
>
> What we might want to consider for the PJSIP Java bindings is to change the
> exception handling there to throw checked exceptions if a "pj::Error" is
> thrown from the C++ code, in order to force users to handle a "pj::Error".
> But this is more or less a separate change that does not really have
> anything to do with my changes.
>
> Thanks again for the quick testing and the quick feedback!
>
>
> Have a nice day and best regards,
>
> Christian
>
>
> On 16/10/18 06:31, Ming wrote:
>
> Hi Christian,
>
> I did some testing here and the exception does work great during runtime
> now!
>
> However, in my opinion, the "throw()" specification is still important
> for SWIG. While it's true that throw() is deprecated for C++, it
> remains a valid (and important) part of other languages such as Java.
> And having throw() in the declaration of the function will help the
> programmers during the development process.
>
> For example, if you try to call a function which throws an exception
> in Java but forget to put try and catch, the compiler will give an
> error such as:
> sample.java:155: error: unreported exception Exception; must be caught
> or declared to be thrown
>
> But it's now gone, so the developer must now carefully check each
> function in order to know whether it can throw an exception.
>
> Best regards,
> Ming
>
> On Mon, Oct 15, 2018 at 10:18 AM, Ming <ming@xxxxxxxxx> wrote:
>
> Hi Christian,
>
> Thank you for the hard work. Let us review and test this. Given the
> scale of the platforms and language binding affected, this may take
> some time. We will update you again.
>
> Best regards,
> Ming
>
>
> On Mon, Oct 15, 2018 at 4:19 AM, Christian Hoff <christian_hoff@xxxxxxx>
> wrote:
>
> Hello all,
>
> during the last weeks, I worked hard on the patch for PJSUA2 and C++ 17
> compatibility again. For those who have forgotten: Currently the PJSUA2
> headers cannot be compiled with C++ 17 because C++ 17 removes support for
> exception specifications. But unfortunately PJSUA2 makes have use of
> exception specifications.
>
> The open issue with my last patch was that SWIG no longer generates catch
> handlers that catch "pj::Error", as Ming pointed out. I have now updated the
> patch so that SWIG also generates the required catch handlers. Doing so has
> been a quite difficult, tedious and time-consuming task. But finally I am
> done and I am happy to share the resulting patch for inclusion into pjsip.
> The updated patch is attached to this mail.
>
> The magic on the SWIG side is done with the "%exception" directive, which
> installs a default global exception handler. In my updated patch, I am now
> using this "%exception" directive to generate the catch handler for
> "pj::Error" (see the new file "pjsip-apps/src/swig/pj_error.i"). The
> rationale behind this was that I really wanted to remove all the "throw
> (pj::Error)" from the header files, so I did not want to have a "PJ_THROW"
> that would expand to nothing during the compile and expand to the old "throw
> (...)" when using SWIG. "throw (...)" is now deprecated syntax and we should
> not keep it around for the sake of SWIG in my opinion.
>
> Some basic testing was also done:
>
> To verify the C++ 17 compatibility, I compiled my application with the
> installed adopted pjsua2 header files.
> To verify the generated SWIG language bindings and catch handlers:
>
> For the Python language binding, I verified that the compile works.
> Afterwards I installed the generated language binding and ran the test file
> "test.py". "test.py" also verifies the exception handling. I also inspected
> the generated catch handlers for pj::Error visually.
> For the Java and C# bindings, I verified that the compile works and
> inspected the generated catch handlers visually. Unfortunately I did not
> find a suitable unit test for the exception handling, but I am optimistic
> that it works nevertheless. I could not run the generated code for the C#
> bindings because I do not have Xamarin installed on my machine. But as I
> said, at least I can compile the C++ wrapper code for C#.
>
> I hope that this patch can now be considered for inclusion into PJSIP.
>
>
> Best regards,
>
> Christian
>
>
> On 25/09/18 10:26, Max Truxa wrote:
>
> Hello Christian,
>
> how about replacing the throw specifications with a macro that expands
> to nothing when compiling as C++ but `throw(Error)` when being parsed
> by SWIG?
>
> Something like this:
>
> #ifdef SWIG // defined if SWIG is parsing the file
> # define PJ_CAN_THROW throw(Error)
> #else
> # define PJ_CAN_THROW
> #endif
>
> struct Foo
> {
> void bar() PJ_CAN_THROW;
> };
>
> Regards,
>
> - Max
>
> On Tue, Sep 25, 2018 at 8:30 AM Christian Hoff <christian_hoff@xxxxxxx>
> wrote:
>
> Hello Ming,
>
> this is also an interesting idea. But will this actually work..? I am
> wondering because SWIG only parses the header files of PJSUA2. However
> the actual function implementation is in the ".cpp" source files and in
> these files you can find the code that throws the exceptions and that
> could use "PJ_THROW()". But to my knowledge SWIG does not parse the
> source files and would thus not know which functions may call
> "PJ_THROW". Or am I wrong about this?
>
>
> Best regards,
>
> Christian
>
>
> On 25/09/18 03:08, Ming wrote:
>
> Hi Christian,
>
> We haven't found a good way either. Our idea is to put PJ_THROW()
> macro to replace throw(), which can somehow be used by SWIG to
> generate the exception specs.
>
> Regards,
> Ming
> On Tue, Sep 25, 2018 at 4:21 AM Christian Hoff <christian_hoff@xxxxxxx>
> wrote:
>
> Hello Ming and all,
>
> I have now done some research into SWIG and exceptions. After reading
> the SWIG documentation, I found out that SWIG indeed parses the C++
> exception specification to figure out for what types of exceptions
> "catch(..)" should be generated (see
> http://www.swig.org/Doc3.0/SWIGDocumentation.html#SWIGPlus_exception_specifications
> ). Exception types that are specified in the exception specification are
> automatically propagated to the higher language (Java, C# etc.). Now
> that we need to remove the exception specification, we lose that
> capability. Thanks to Ming for the hint. However there is the
> possibility to adapt the SWIG interface file
> ("pjsip-apps/src/swig/pjsua2.i") and to add explicit "%catches"
> statements for each method that describe which types of exceptions the
> method could possibility throw. So I guess I need to rework my patch for
> this. I just hope that I will find some way to avoid having to annotate
> every method with "%catches".... I'll see. Or if someone knows how to do
> it, then I would be very grateful for a reply.
>
>
> Best regards,
>
> Christian
>
>
> On 24/09/18 12:43, Christian Hoff wrote:
>
> Hello Ming,
>
> thanks for your reply! I will have a look into the SWIG topic this
> evening and will let you know my findings.
>
>
> Best regards,
>
> Christian
>
>
> On 24/09/18 09:03, Ming wrote:
>
> Hi Christian,
>
> We apologize for the delay in response. We're currently reviewing it
> and have also managed to do some testing. While for C/C++, this seems
> to be okay, we're still considering how to best handle this for our
> Android and Java (via SWIG binding), which may still need the "throw"
> specifications in order for the developers to know that the functions
> may throw some exceptions.
>
> If you have any feedback regarding this, please let us know.
>
> Best regards,
> Ming
>
> On Mon, Sep 24, 2018 at 2:32 PM Christian Hoff
> <christian_hoff@xxxxxxx> wrote:
>
> Hello all,
>
> some feedback for this patch is still appreciated. In general I am
> getting somewhat frustrated about the collaboration with people who
> want
> to contribute to the project. Already in the past other people have
> posted patches here to this list, but did not get any response - not
> even a response what needs to be changed for the patch to be
> accepted. I
> think this is not a good mode of collaboration. Everyone who posts a
> patch here to this mailing list has done this with the intention to
> help
> the project PJSIP to move along (and has spent much time developing the
> patch), so you should at least return the favour and review the patch.
>
> This particular issue that I am trying to address with my patch (that
> the PJSUA2 headers don't compile against C++17) is an issue that you
> will eventually need to fix anyhow, so why not fix it right now? As of
> now, modern versions of GCC use C++ 14 by default, it is only a matter
> of time until C++ 17 will become the default and people will start
> complaining that there are compile errors from the PJSUA2 headers.
>
> Besides this, my own app uses PJSUA2 with C++ 17 features, so I would
> have to revert back to C++14 if this patch is not accepted upstream.
> This would be really unnecessary. So I would really appreciate a patch
> review.
>
> Best regards,
>
> Christian
>
>
> On 18/09/18 20:57, Christian Hoff wrote:
>
> Hello all,
> Hello Ming & Riza,
>
> do you have any feedback regarding this patch for C++ 17 support? I am
> also willing to rework it if required, but some feedback would be
> nice.
>
>
> Thanks in advance and kind regards,
>
> Christian
>
>
> On 12/09/18 18:23, Christian Hoff wrote:
>
> Hello all,
>
> currently it is not possible to compile a pjsua2 application with
> C++17. The reason is that C++17 removes support for dynamic exception
> specifications (that is "throws ..." declarations at the end of a
> function). Unfortunately PJSUA2 makes heavy use of exception
> specifications and most PJSUA2 functions are declared as "throw
> (Error)" - meaning that they can throw an exception of type "Error"
> that the programmer is supposed to catch.
>
> The consequence is that a lot of compile errors can be observed when
> compiling a pjsua2 application with C++17. The compile errors look as
> follows:
>
> /usr/local/include/pjsua2/json.hpp:80:33: error: ISO C++17 does not
> allow dynamic exception specifications
> virtual string saveString() throw(Error);
> ^~~~~
>
> The compile of the same application worked fine when using the C++14
> standard, but it got broken when I switched from C++14 to the C++17
> standard.
>
> See
> https://www.bfilipek.com/2017/05/cpp17-details-fixes-deprecation.html#removing-deprecated-exception-specifications-from-c17
>
> and
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0003r0.html
> for more background about the removal of exception specifications.
>
> To make the application compile again, I had to remove "throw(Error)"
> in the whole PJSUA2 source code and include files. This seems to be
> the only viable and good fix for this issue. I wrote my own "sed"
> script to do that and made some cosmetic corrections afterwards. Then
> I generated the attached patch file. Could you please apply this
> patch to the code base or give me feedback what I still need to
> improve? I verified that pjsip compiles fine with the patch applied.
>
> Thank you very much!
>
>
> Best regards,
>
> Christian
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
>
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
>
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
>
>
>
> _______________________________________________
> Visit our blog: http://blog.pjsip.org
>
> pjsip mailing list
> pjsip@xxxxxxxxxxxxxxx
> http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
>
_______________________________________________
Visit our blog: http://blog.pjsip.org
pjsip mailing list
pjsip@xxxxxxxxxxxxxxx
http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org
