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

support of noClassPath in getFields #780

Merged
merged 4 commits into from
Aug 23, 2016
Merged

support of noClassPath in getFields #780

merged 4 commits into from
Aug 23, 2016

Conversation

arturbosch
Copy link
Contributor

…hen the parent class is not found in noClassPath-Mode

This tries to fix #775.

…hen the parent class is not found in noClassPath-Mode
@monperrus
Copy link
Collaborator

thanks for this pull request!

this changes the intended fail-fast semantics of getActualClass. For #755, I would rather go for an appropriate try/catch in getAllFields.

@arturbosch
Copy link
Contributor Author

Something like that?

    @Override
    public Collection<CtFieldReference<?>> getAllFields() {
        CtType<?> t = getDeclaration();
        try {
            if (t == null) {
                return RtHelper.getAllFields(getActualClass(), getFactory());
            } else {
                return t.getAllFields();
            }
        } catch (SpoonClassNotFoundException cnfe) {
            return handleParentNotFound(cnfe);
        }
    }

    private Collection<CtFieldReference<?>> handleParentNotFound(SpoonClassNotFoundException cnfe) {
        String msg = "cannot load class: " + getQualifiedName() + " with class loader "
                + Thread.currentThread().getContextClassLoader();
        if (getFactory().getEnvironment().getNoClasspath()) {
            // should not be thrown in 'noClasspath' environment (#775)
            Launcher.LOGGER.warn(msg);
            return Collections.emptyList();
        } else {
            throw cnfe;
        }
    }

Maybe you have a suggestion for a better message name. Cheers.

@arturbosch
Copy link
Contributor Author

Furthermore getDeclaredFields has the same behaviour and maybe needs a try-catch too.

But now getXXXFields will always return an empty list even if 'Foo' has a field.

public class Foo extends Unknown {
    private String s;
...
}

@monperrus
Copy link
Collaborator

Something like that?

exactly. should the try/catch be around the RtHelper call directly?

@monperrus monperrus changed the title Change CtTypeReferenceImpls getFields method to return an emptyList w… support of noClassPath in getFields Aug 10, 2016
@msteinbeck
Copy link
Contributor

Is it worth merging now?

@@ -53,11 +55,17 @@ public void test() throws Exception {
}
assertNull(superclass.getDeclaration());

Collection<CtFieldReference<?>> allFields = superclass.getAllFields();
System.out.println(allFields);
System.out.println(superclass.getDeclaredFields());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove these sout?

@arturbosch
Copy link
Contributor Author

I incorporated all of your feedback. Sorry, I was in a hurry last time.

@GerardPaligot
Copy link
Contributor

Thanks @arturbosch for your last commit but it seems that you removed your call to superclass.getAllFields();.

Can you add an assert on this call to show we don't throw anymore an exception? After that, we'll merge your PR.

@arturbosch
Copy link
Contributor Author

Done. Is the comment ok or should I remove it ;) ?

@GerardPaligot
Copy link
Contributor

Thanks @arturbosch! :)

@GerardPaligot GerardPaligot merged commit eed8a79 into INRIA:master Aug 23, 2016
@tdurieux tdurieux mentioned this pull request Sep 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CtTypeImpl.getAllFields may throw a SpoonClassNotFoundException
4 participants