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

Final audit before 1.0 #148

Closed
estk opened this issue Mar 11, 2020 · 9 comments · Fixed by #154
Closed

Final audit before 1.0 #148

estk opened this issue Mar 11, 2020 · 9 comments · Fixed by #154
Labels
Milestone

Comments

@estk
Copy link
Owner

estk commented Mar 11, 2020

Last task before release

@estk estk added this to the 1.0 milestone Mar 11, 2020
@estk estk added the chore label Mar 12, 2020
@estk
Copy link
Owner Author

estk commented Apr 4, 2020

@gadunga wanna pair on this?

@estk estk changed the title Review docs before release 1.0 Final audit before 1.0 Apr 4, 2020
@estk estk mentioned this issue Apr 4, 2020
15 tasks
@gadunga
Copy link
Collaborator

gadunga commented Apr 4, 2020

Sure, I can take a look. I'd like to try and get #21 in as well. What do you think?

@estk
Copy link
Owner Author

estk commented Apr 4, 2020

Ya that sounds good.

@gadunga
Copy link
Collaborator

gadunga commented Apr 4, 2020

Made a comment and opened an MR to remove log4rs::FormatError::XmlFeatureFlagRequired.

@rakshith-ravi
Copy link

Just a suggestion, but can we remove all Box<dyn Trait> instances and make them as generic parameters instead? Boxs get allocated in the heap and heap access is almost always slower than stack access. So it would significantly improve performance as well as help clean up the code (now that we don't have to do all those Box::news any more). Generic parameters make it a lot more easier to have ownership of the values as well.

@estk
Copy link
Owner Author

estk commented Jul 22, 2020

@rakshith-ravi I take your point, and I've thought about it myself. I worry that with such a large change to the config api we would split the ecosystem of log4rs users and plugins.

@rakshith-ravi
Copy link

Well my thinking was that before we hit 1.0 is the best chance for us to make any breaking changes. Once we hit stable 1.0, it'll become difficult to undo these and we wouldn't want it to become a performance bottleneck because of such a silly thing, which we could've changed

@estk
Copy link
Owner Author

estk commented Sep 28, 2020

@rakshith-ravi I spent some time investigating this, there's just no way to do it since we need to hold heterogeneous collections of Append and Encode impling types. The reason we cant just hard code these is because the api is extensible and we dont know all possible Append etc types up front.

@rakshith-ravi
Copy link

Ahh, I suppose that makes sense. Generics don't really work out when we need a collection of multiple items of different size. Looks like Box is our only way to go. Thanks for considering it and attempting to fix it through :D

@estk estk closed this as completed in #154 Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants