Object Reference Validation #2133 #2144
Conversation
| // Do we care about how many times each edge is used, or should we just return as soon as | ||
| // we find one that is used more than once? |
There was a problem hiding this comment.
Experience has shown that early-aborting validation checks is generally a bad idea (which is why they're done in the background through a service, instead of using the normal Rust Result-based error handling).
With early abort, you often end up with not enough information to figure out what's going wrong.
| @@ -62,6 +62,12 @@ impl<S: State> Deref for Service<S> { | |||
| } | |||
| } | |||
|
|
|||
| impl<S: State> DerefMut for Service<S> { | |||
There was a problem hiding this comment.
Added this so that I can clear out validation errors in tests after checking for the one we care about, i.e. services.validation.errors.clear(); instead of having to create an object graph that has no other validation errors. Let me know if there is a better way to accomplish this.
There was a problem hiding this comment.
It feels wrong, but I don't have a better idea right now. Since it's only used in tests, we should slap a #[cfg(test)] on there, to make sure it's not misused by other code.
| referenced_cycles.insert(c.clone(), { | ||
| if let Some(count) = referenced_cycles.get(c) { | ||
| count + 1 | ||
| } else { | ||
| 1 | ||
| } | ||
| }); |
There was a problem hiding this comment.
I still haven't done a real review (as I said over in #2133, I'm on vacation right now), but this jumped out to me.
To be clear, this is fine, I would merge it as-is. I just wanted to mention, for the sake of education, that this looks like the perfect use case for the entry API. Something like this should work (not compiled or tested):
| referenced_cycles.insert(c.clone(), { | |
| if let Some(count) = referenced_cycles.get(c) { | |
| count + 1 | |
| } else { | |
| 1 | |
| } | |
| }); | |
| *referenced_cycles.entry(c.clone()).or_insert(0) += 1; |
Feel free to take this suggestion, or not, as you see fit.
|
Ah yes, that looks like the right way to do this. I will add it to the cleanup list! I’m generally familiar with rust syntax, but still learning the finer details so happy to consider any suggestions you might have.On Dec 18, 2023, at 6:34 AM, Hanno Braun ***@***.***> wrote:
@hannobraun commented on this pull request.
In crates/fj-core/src/validate/sketch.rs:
+ referenced_cycles.insert(c.clone(), {
+ if let Some(count) = referenced_cycles.get(c) {
+ count + 1
+ } else {
+ 1
+ }
+ });
I still haven't done a real review (as I said over in #2133, I'm on vacation right now), but this jumped out to me.
To be clear, this is fine, I would merge it as-is. I just wanted to mention, for the sake of education, that this looks like the perfect use case for the entry API. Something like this should work (not compiled or tested):
⬇️ Suggested change
- referenced_cycles.insert(c.clone(), {
- if let Some(count) = referenced_cycles.get(c) {
- count + 1
- } else {
- 1
- }
- });
+ *referenced_cycles.entry(c.clone()).or_insert(0) += 1;
Feel free to take this suggestion, or not, as you see fit.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
nathan-folsom
force-pushed
the
object-ownership-validation
branch
from
5ad1fdc
to
d678391
Compare
|
@hannobraun Since you last looked, did some cleanup and included referenced + referencing objects in error messages. Feels merge-able to me, but let me know what you think when you have the chance! |
|
Thank you for the updates, @nathan-folsom! I am now confident that I won't review this before January. In addition to being on vacation, I've also picked up a cold on Christmas. Little motivation to do anything but watch movies right now 😄 Sorry for the delay. Usually I handle these within a few days, at most. |
|
No problem, hope you're feeling better! |
|
I am, thank you! Getting closer to where I can review this. |
There was a problem hiding this comment.
Hey @nathan-folsom, thank you again for the pull request. And I'm very sorry it took me so long to get to it. There was a confluence of factors, but I'm finally back in the game now.
I've left some comments. The only problem I'd like to see addressed, is the #[cfg(test)] for the DerefMut implementation, since that could be error-prone, if left as-is. I'd be happy to merge the pull request with the typo remaining (it can be fixed later, and I'd rather get this merged as soon as practical).
One note, since this confused me for a bit: My original list in #2133 says we need to validate (on the Sketch level) that each Region is referenced by exactly one Sketch. This pull request doesn't do that, which is actually correct! There's only one Sketch, and it references its regions via one ObjectSet, which already guarantees uniqueness. So all good!
| } | ||
|
|
||
| impl SketchValidationError { | ||
| fn check_obect_references( |
There was a problem hiding this comment.
| fn check_obect_references( | |
| fn check_object_references( |
(Also requires changing the caller, of course, so just clicking "accept" on this suggestion won't be enough.)
| @@ -62,6 +62,12 @@ impl<S: State> Deref for Service<S> { | |||
| } | |||
| } | |||
|
|
|||
| impl<S: State> DerefMut for Service<S> { | |||
There was a problem hiding this comment.
It feels wrong, but I don't have a better idea right now. Since it's only used in tests, we should slap a #[cfg(test)] on there, to make sure it's not misused by other code.
nathan-folsom
force-pushed
the
object-ownership-validation
branch
from
9efe374
to
5e885a4
Compare
|
@hannobraun Done and done! |
hannobraun
force-pushed
the
object-ownership-validation
branch
from
789ac99
to
cdcf015
Compare
Add validation of object references in Sketches and Solids as outlined in #2133