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

Add Rc<T> and Arc<T> sources #340

Merged
merged 2 commits into from
Feb 10, 2024
Merged

Add Rc<T> and Arc<T> sources #340

merged 2 commits into from
Feb 10, 2024

Conversation

InfiniteCoder01
Copy link

No description provided.

@jeertmans
Copy link
Collaborator

Nice @InfiniteCoder01! Running the checks, can you update the code to make all tests pas?

@InfiniteCoder01
Copy link
Author

@jeertmans, sorry for not reading feature list properly. Now I've fixed it (I don't think Rc and Arc appear in nostd environment)

@jeertmans
Copy link
Collaborator

Thanks @InfiniteCoder01! Could you please add one or two tests, and a docstring on top of each class, to show very basic examples?

@InfiniteCoder01
Copy link
Author

I don't see any tests for implementation of Source trait for str and [u8]. Could you give me an example?
As for docs, I didn't add any structs/types, just implemented some traits.

@jeertmans
Copy link
Collaborator

@InfiniteCoder01 Did you check https://github.com/maciejhirsz/logos/blob/master/logos/src/source.rs?

It contains implementation of Source for both str and [u8].

@InfiniteCoder01
Copy link
Author

Those are implementations, but there are no docs, nor tests for them (docs only for Source trait). But there is a module-level doc comment where I've added new sources.

@jeertmans
Copy link
Collaborator

True, but that's not because there is no documentation at the moment that it isn't worth adding some :-)

@InfiniteCoder01
Copy link
Author

In rust, you can't add docs to impl Trait and it's methods. But I'll add some tests then

@InfiniteCoder01
Copy link
Author

Can't really add tests until #341

@jeertmans
Copy link
Collaborator

oh yeah, you've got a good point!

Looking at your code, I actually wonder if it just makes sense to rather implement Source for AsRef<T: impl Source>?

@ikrivosheev
Copy link

@InfiniteCoder01 hello! It will be cool to add Box.

Copy link
Contributor

@marcospb19 marcospb19 left a comment

Choose a reason for hiding this comment

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

I implemented this for Rc and Arc using a single impl block:

//! * `Source` - implemented by default for `&str`, `&[u8]` and wrapper types, used by the `Lexer`.

...

#[cfg(feature = "std")]
impl<T> Source for T
where
    T: Deref,
    <T as Deref>::Target: Source,
{
    type Slice<'a> = <T::Target as Source>::Slice<'a>
        where T: 'a;

    fn len(&self) -> usize {
        self.deref().len()
    }

    fn read<'a, Chunk>(&'a self, offset: usize) -> Option<Chunk>
    where
        Chunk: self::Chunk<'a>,
    {
        self.deref().read(offset)
    }
    
    ...
}

@InfiniteCoder01 hello! It will be cool to add Box.

Box also implements Deref, so the impl above covers it too.

@marcospb19
Copy link
Contributor

marcospb19 commented Feb 8, 2024

I tried using AsRef instead of Deref but couldn't.

impl<T, Inner> Source for T
where
    T: AsRef<Inner>,
    Inner: Source,
{
error[E0207]: the type parameter `Inner` is not constrained by the impl trait, self type, or predicates

Deref uses associated types and AsRef doesn't, I think that makes it impossible to implement this for AsRef.

@jeertmans
Copy link
Collaborator

Makes sense, thank you! @InfiniteCoder01 Could you please update your branch with respect to the master and resolve conflicts? So I can merge it :)

@InfiniteCoder01
Copy link
Author

Sorry for closing the PR, I'm still bad at git stuff. Should work now, tested with cargo test and cargo hack check --feature-powerset --no-dev-deps

@jeertmans
Copy link
Collaborator

No issue, thanks for replying so quickly @InfiniteCoder01! Running CI, it seems that you should run cargo fmt on your code :-)

@jeertmans jeertmans merged commit ba69cc3 into maciejhirsz:master Feb 10, 2024
18 checks passed
@jeertmans
Copy link
Collaborator

Thank you for your contribution!

akrantz01 referenced this pull request in akrantz01/antsi Aug 2, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [logos](https://logos.maciej.codes/)
([source](https://togithub.com/maciejhirsz/logos)) | dependencies |
patch | `0.14.0` -> `0.14.1` |

---

### Release Notes

<details>
<summary>maciejhirsz/logos (logos)</summary>

###
[`v0.14.1`](https://togithub.com/maciejhirsz/logos/releases/tag/v0.14.1):
0.14.1 - Debug feature and fixes

#### What's Changed

- fix(doc): reset logos2 to logos by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/372](https://togithub.com/maciejhirsz/logos/pull/372)
- chore(book): add JSON-borrowed parser example by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/373](https://togithub.com/maciejhirsz/logos/pull/373)
- Add Rc<T> and Arc<T> sources by
[@&#8203;InfiniteCoder01](https://togithub.com/InfiniteCoder01) in
[https://github.com/maciejhirsz/logos/pull/340](https://togithub.com/maciejhirsz/logos/pull/340)
- Fix unicode dot by [@&#8203;RustyYato](https://togithub.com/RustyYato)
in
[https://github.com/maciejhirsz/logos/pull/376](https://togithub.com/maciejhirsz/logos/pull/376)
- chore(docs): cleanup examples by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/381](https://togithub.com/maciejhirsz/logos/pull/381)
- chore(lib): add debug feature by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/382](https://togithub.com/maciejhirsz/logos/pull/382)
- Cleanup unused Source features by
[@&#8203;kmicklas](https://togithub.com/kmicklas) in
[https://github.com/maciejhirsz/logos/pull/335](https://togithub.com/maciejhirsz/logos/pull/335)
- chore(deps): bump peaceiris/actions-mdbook from 1 to 2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/maciejhirsz/logos/pull/387](https://togithub.com/maciejhirsz/logos/pull/387)
- Fix `Lexer::clone` leak and UB + tests by
[@&#8203;Jakobeha](https://togithub.com/Jakobeha) in
[https://github.com/maciejhirsz/logos/pull/390](https://togithub.com/maciejhirsz/logos/pull/390)
- fix(lib): correctly handle miss for loop in loop by
[@&#8203;lukas-code](https://togithub.com/lukas-code) in
[https://github.com/maciejhirsz/logos/pull/393](https://togithub.com/maciejhirsz/logos/pull/393)
- chore(lib): remove error branch from LUT if it is unreachable by
[@&#8203;RustyYato](https://togithub.com/RustyYato) in
[https://github.com/maciejhirsz/logos/pull/386](https://togithub.com/maciejhirsz/logos/pull/386)
- fix(docs): typo by
[@&#8203;joerivanruth](https://togithub.com/joerivanruth) in
[https://github.com/maciejhirsz/logos/pull/396](https://togithub.com/maciejhirsz/logos/pull/396)
- chore(docs): Adds graph debug documentation to book by
[@&#8203;afreeland](https://togithub.com/afreeland) in
[https://github.com/maciejhirsz/logos/pull/379](https://togithub.com/maciejhirsz/logos/pull/379)
- chore: drop python linting frmo pre-commit-config by
[@&#8203;LeoDog896](https://togithub.com/LeoDog896) in
[https://github.com/maciejhirsz/logos/pull/403](https://togithub.com/maciejhirsz/logos/pull/403)
- refactor: don't use deprecated max_value() method by
[@&#8203;LeoDog896](https://togithub.com/LeoDog896) in
[https://github.com/maciejhirsz/logos/pull/404](https://togithub.com/maciejhirsz/logos/pull/404)
- chore(version): bump logos version to 0.14.1 by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/409](https://togithub.com/maciejhirsz/logos/pull/409)
- fix(docs): change old 0.14.0 by
[@&#8203;jeertmans](https://togithub.com/jeertmans) in
[https://github.com/maciejhirsz/logos/pull/410](https://togithub.com/maciejhirsz/logos/pull/410)

#### New Contributors

- [@&#8203;InfiniteCoder01](https://togithub.com/InfiniteCoder01) made
their first contribution in
[https://github.com/maciejhirsz/logos/pull/340](https://togithub.com/maciejhirsz/logos/pull/340)
- [@&#8203;RustyYato](https://togithub.com/RustyYato) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/376](https://togithub.com/maciejhirsz/logos/pull/376)
- [@&#8203;Jakobeha](https://togithub.com/Jakobeha) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/390](https://togithub.com/maciejhirsz/logos/pull/390)
- [@&#8203;lukas-code](https://togithub.com/lukas-code) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/393](https://togithub.com/maciejhirsz/logos/pull/393)
- [@&#8203;joerivanruth](https://togithub.com/joerivanruth) made their
first contribution in
[https://github.com/maciejhirsz/logos/pull/396](https://togithub.com/maciejhirsz/logos/pull/396)
- [@&#8203;afreeland](https://togithub.com/afreeland) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/379](https://togithub.com/maciejhirsz/logos/pull/379)
- [@&#8203;LeoDog896](https://togithub.com/LeoDog896) made their first
contribution in
[https://github.com/maciejhirsz/logos/pull/403](https://togithub.com/maciejhirsz/logos/pull/403)

**Full Changelog**:
maciejhirsz/logos@v0.14...v0.14.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/akrantz01/antsi).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

4 participants