Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

This contains a test to prove that Parrot_api_get_compiler is unreliable #780

Closed
wants to merge 5 commits into from

5 participants

@bdw
bdw commented

The return value of Parrot_api_get_compiler is unreliable (i.e. allways returns 1). This test proves that. Maybe compreg should raise an error?

@Benabik
Collaborator

I think you have to use imcc_get_pir_compreg_api(interp, 1, &pir_compiler) (where pir_compiler is a Parrot_PMC) before compreg 'PIR' will work. Check out setup_imcc() in frontend/parrot2/main.c:191

@bdw
bdw commented

Thats very true but not the point.
The point is that Parrot_api_get_compiler returns true for /all/ calls, irrespective of whether I put in a reasonable name or garbage. Personally, I think not finding a compiler should be a failure case.

@Whiteknight

The return value of API calls indicates whether the operation in question completed normally or abnormally (an unhandled exception was thrown). If the return value is 0, that means there is an unhandled exception stored in the interpreter to be examined and possibly reported to the user.

Neither of the compreg get/set pathways throw exceptions, so we would expect this API to always return 1. In the future if we use a different mechanism that throws exceptions, we might expect this behavior to change.

The Parrot_api_get_compiler routine returns a PMCNULL in an out parameter if the compiler is not found. You can inspect that value to determine if a compiler has been found.

That said, the current behavior is sane and consistent with the rest of the API. The case can be made that a user can legitimately register PMCNULL to the compreg hash, and this creates a semipredicate problem. The argument can be made that the more useful behavior might be for Parrot_api_get_compreg to throw an exception if a compiler is not found, instead of returning PMCNULL.

In the absence of thrown exceptions, the current behavior is "correct"

@bdw
bdw commented

I understand that this is not a bug per se. But from the point of view of an API user, not having found a compiler is a failure. The result of Parrot_api_get_compiler is useless as the function will never fail. In fact, having to check for the return value being NULL aside from having to check the return value is redundant.
It could probably be resolved by throwing an exception inside the API function. That way, existing code (that does not expect compreg to fail) will continue to work, and the API will do what the user expects.

@zhuomingliang
Collaborator

Whiteknight, how about this pull request?

@Whiteknight

I am +1 to merging this, but it does not appear mergable. I've tried twice and it doesn't seem like I can merge it (or merging brings no changes). Can somebody please verify?

@bdw
@bdw

Oh, that is my fault. I have erased the repository only to start again with a new one.
I still have the files locally, and I can push a new branch if needed. But that does mean a new pull request.
Also, I should add imcc_get_pir_compiler to the test first.

@leto leto closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 3, 2012
  1. @bdw

    Typo in api.t

    bdw authored
Commits on Jun 4, 2012
  1. @bdw
Commits on Jun 5, 2012
  1. @bdw
Commits on Jun 6, 2012
  1. Merge remote-tracking branch 'upstream/master'

    Bart Wiegmans authored
Commits on Jun 8, 2012
  1. @bdw
This page is out of date. Refresh to see the latest.
Showing with 34 additions and 1 deletion.
  1. +34 −1 t/src/embed/api.t
View
35 t/src/embed/api.t
@@ -14,7 +14,7 @@ my $parrot_config = "parrot_config" . $PConfig{o};
plan skip_all => 'src/parrot_config.o does not exist' unless -e catfile("src", $parrot_config);
-plan tests => 8;
+plan tests => 9;
=head1 NAME
@@ -388,6 +388,39 @@ executed MyMethod
executed MyMethod
OUTPUT
+
+c_output_is( linedirective(__LINE__) . <<"CODE", << 'OUTPUT', "Parrot_api_get_compiler" );
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "parrot/api.h"
+
+int main(void) {
+ Parrot_PMC interp;
+ Parrot_PMC pir_compiler, foobar_compiler;
+ Parrot_String pir_s, foobar_s;
+
+ Parrot_api_make_interpreter(NULL, 0, NULL, &interp);
+ Parrot_api_string_import_ascii(interp, "PIR", &pir_s);
+ if(!Parrot_api_get_compiler(interp, pir_s, &pir_compiler)) {
+ puts("I could not get the PIR compiler");
+ } else {
+ puts("I got the PIR compiler");
+ }
+ Parrot_api_string_import_ascii(interp, "foobar", &foobar_s);
+ if(!Parrot_api_get_compiler(interp, foobar_s, &foobar_compiler)) {
+ puts("I could not get the foobar compiler");
+ } else {
+ puts("I got the foobar compiler");
+ }
+ exit(0);
+}
+CODE
+I got the PIR compiler
+I did not get the foobar compiler
+OUTPUT
+
+
# Local Variables:
# mode: cperl
# cperl-indent-level: 4
Something went wrong with that request. Please try again.