You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/05/20 02:24:16 UTC

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3747: mm:initialize ensure alignment.

xiaoxiang781216 commented on pull request #3747:
URL: https://github.com/apache/incubator-nuttx/pull/3747#issuecomment-844633152


   The change looks good.
   
   > I walked through this with @davids5. I agree with the fix.
   > 
   > This change broke things:
   > [635cfad](https://github.com/apache/incubator-nuttx/commit/635cfadc2598f14d7cac4482acd9ed85d5dbe5ac)
   > 
   > What I find interesting about this change is that @davids5 raised a valid criticism about the change needing a better explanation and yet it was approved without even a response to his comment.
   > 
   
   I think @davids5's concern is addressed before merging: both background info and ASAN heap manager is provided in PR.
   
   > Changes effecting such a critical piece of memory should get a more thorough review than a single reviewer.
   
   Four people join the review in 3 days and get approved before merging. The issue is hard to find in our environment since most link scripts(include ours) add algin(4)/align(8) after .bss.
   Anyway, thank you and @davids5 spending the time to find the root cause. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org