#rust-internals

/

      • cods joined the channel
      • GBGamer is now known as
      • skade_ has quit
      • skade joined the channel
      • eibwen has quit
      • dashed joined the channel
      • niconii has quit
      • futile joined the channel
      • laumann joined the channel
      • mw joined the channel
      • bjz joined the channel
      • yati joined the channel
      • yati has quit
      • defuz has quit
      • RC9 joined the channel
      • eibwen joined the channel
      • vikingofrock joined the channel
      • vikingofrock
        So I'm working on an issue for Clippy, and I was wondering if I could get some help with understanding Spans
      • sigma joined the channel
      • I wanted to get the span for .map(foo).underline(bar) in opt.map(foo).underline(bar)
      • but I can't figure out how to do that
      • *underline -> unwrap_or
      • I want to have a note that suggests replacing '.map(foo).unwrap_or(bar)' with 'map_or(bar, foo)'
      • mw
        vikingofrock: do you have the spans of map(foo) and unwrap_or(bar)?
      • skeuomor1 joined the channel
      • Ms2ger joined the channel
      • vikingofrock
        yeah
      • is there a way to combine them?
      • skeuomorf has quit
      • mw
        every Span value has a `hi` and a `lo` field
      • these are the starting and end positions (in bytes) within the codemap
      • so if you can create a new span with min(sp1.lo, sp2.lo) and max(sp1.hi, sp2.hi) to get a combined span
      • there is syntax::codemap::mk_sp() for creating new Span values
      • imperio joined the channel
      • huon
        (NB. for truly correct handling, you probably only want to emit a warning/combine the spans if they have the same expansion info, or there'll be an error the user can't fix for something like `some_macro!(foo).unwrap_or(bar)`, where `some_macro!` expands to have a .map(...) at the end.)
      • I guess clippy may want/have utilties that do the checks already, e.g. `fn combine_adjacent_spans(s1: Span, s2: Span) -> Option<Span>`
      • vikingofrock
        thanks
      • that's really helpful
      • mw
        combine_adjacent_spans() sounds exactly like what you need
      • huon
        (I don't know if the function exists... but it totally should if not)
      • mw
        huon: :) oh
      • vikingofrock
        Hmm I'm not seeing any convenient function like that
      • what is expansion info?
      • mw
        it has to do with macro expansion
      • huon
      • vikingofrock
        thanks
      • huon
        so combine_adjacent_spans could look something like if s1.expn_id == s2.expn_id { None } else { Some(mk_sp(min(s1.lo, s2.lo), max(s1.hi, s2.hi)))) }
      • mw
        that should be !=, right?
      • huon
        err, whoops, the if/else arms should be swapped
      • yeah
      • eap joined the channel
      • vikingofrock
        cool cool
      • so when would the expn_ids be different for neighboring methods like that?
      • TimNN has quit
      • is it in some macro expansion edge case?
      • huon
        the some_macro!(foo).unwrap_or(bar) macro example above
      • vikingofrock
        oh, somehow I missed that
      • gotcha
      • huon
        if `some_macro!(foo)` expands to `foo.map(baz)`, the expn_ids/spans of the `map` will be different
      • futile has quit
      • imperio
        when will be the libs team reunion ?
      • Manishearth: ping
      • nagisa_ joined the channel
      • TimNN joined the channel
      • vikingofrock
        submitted the patch! Thanks so much guys
      • mw
        vikingofrock: you're welcome :)
      • vikingofrock
        =)
      • gleb joined the channel
      • vikingofrock has quit
      • sepp2k joined the channel
      • panicbit has quit
      • panicbit joined the channel
      • Manishearth
        imperio: pong
      • imperio
        Manishearth: about your last comment on gh29976
      • [o__o]
        Add E0492 and E0498 errors explanation: https://github.com/rust-lang/rust/pull/29976
      • imperio
        do I leave the unsafe example or not ?
      • or do I just set the "safe" example ?
      • skade has quit
      • skade joined the channel
      • nagisa_ helicopters his fist at nrc for suggesting `to_owned` and not `into`
      • nagisa_ is now known as
      • huon
        what's better about into?
      • swgillespie has quit
      • nagisa
        its shorter and general-er
      • huon
        and requires more type annotations...
      • oli_obk_
        steveklabnik suggests `String::from("abc")`, acrichto suggests `"abc".to_string()` and clippy suggests `"abc".to_owned()`
      • nagisa
        huon: same can be said about to_string → to_owned
      • huon dislikes String::from(...): it is too long
      • huon
        nagisa: not for strings
      • i.e. "foo".to_owned() always works
      • oli_obk_
        `to_string` is the version mainly used in rustc, while `String::from` is more often used in the docs
      • huon
        :(
      • oli_obk_
        the argument is, that `to_string` more explicitly states what you want to have as a result, and once we get specialization, it'll be just as efficient as `to_owned`
      • huon
        right
      • although the usage in rustc is mainly historical
      • TimNN has quit
      • oli_obk_
        huon: I tried to "fix" it once, and my PR was denied for the reason I just stated
      • huon
        a pile of uses collected via history, such that it's apparently not worth taking a large patch to fix them
      • i.e. the noise from landing it (bitrotting other patches) wasn't worth it in the rejector's mind
      • skade has quit
      • Manishearth
        imperio: leave it out I guess
      • imperio
        Yes sir !
      • Manishearth
        imperio: or just *mention* that once can achieve it unsafely via a wrapper with an unsafe impl of Sync
      • but don't give details
      • imperio
        well, I like to give a really complete explanation :)
      • Manishearth
        imperio: in some cases we should avoid giving people guns to shoot themselves in the foot with :)
      • WHile I agree that diagnostics should take into account edge cases where such solutions may be necessary
      • and help for those cases too
      • imperio
        hum...
      • Manishearth
        I don't think we should be thorough about it
      • give them enough hints to find the gun, don't just give it to them on a silver platter
      • imperio
        I prefer give a gun to someone and explain it why he shouldn't use it rather than he builds it randomly and let him shoot his own foot
      • skade joined the channel
      • eibwen has quit
      • skade has quit
      • m4rw3r joined the channel
      • eibwen joined the channel
      • RC9 has quit
      • Rym joined the channel
      • Manishearth
        imperio: here, the act of learning how to build the gun teaches you how not to shoot it :)
      • waffles has quit
      • waffles joined the channel
      • futile joined the channel
      • nagisa
        holding C backwards ensures best results.
      • sigma has quit
      • jincreator joined the channel
      • dashed has quit
      • skade joined the channel
      • imperio
        Manishearth: also, when you talk about using Mutex instead of Cell, do you mean to wrap the Mutex inside the Cell or replacing the Cell by the Mutex ?
      • edb
        the latter
      • you can't really combine interior mutability schemes
      • imperio
        edb: well okay, thanks
      • edb
        not only is it redundant, but it just doesn't work
      • imperio
        edb: but then we have the issue of "statics cannot have destructor"
      • even if we wrap it
      • edb
        ughrgh
      • imperio
        :)
      • edb
        imperio: wait, what, did anyone suggest putting Mutex in a static?
      • imperio
        I could use lazy_static but I don't think I should
      • edb
        the destructors are not the issue here
      • imperio
      • edb
        the issue is that there's a Box in Mutex
      • imperio
        I gave an unsafe example and Manishearth asked me to give a safe one
      • edb
        I believe nagisa might know about the exact issue describing this
      • imperio: AtomicUsize
      • but it's pretty limited
      • imperio
        the issue is that it's too specific
      • edb
        or AtomicPtr (but you can't do anything with it without touching unsafe code)
      • imperio
        I can't give something this specific here