search

carbon-language / carbon-lang

Carbon Language's main repository: documents, design, implementation, and related tools. (NOTE: Carbon Language is experimental; see README)
http://docs.carbon-lang.dev/
Other
32.24k stars 1.48k forks source link

debug-info: Add checks for instructions without source locations #4251

Closed dwblaikie closed 1 month ago

dwblaikie commented 1 month ago

I think this is what you were looking for, @jonmeow? (I was trying to figure out how to tersely phrase the check to be "this isn't supported here yet, and not clear if it needs to be supported here - but if this assert is hit, then we should add support here?" - open to wording suggestions)

(this will fail testing due to #4249 - so shouldn't be submitted before that, though they aren't textually/patch-ually dependent on each other)

jonmeow commented 1 month ago

@dwblaikie Can you please fix your commit message to not @ me? I will already get notifications from PRs, I don't need notifications from individual commits too. i.e., please fix: https://github.com/dwblaikie/carbon-lang/commit/8134dfb13167427b05dbdaf4a9da691db4625b24

I would generally encourage using PR comments to @ people rather than commits, since the PR is where discussion will occur.

jonmeow commented 1 month ago

BTW, per josh11b at https://github.com/carbon-language/carbon-lang/pull/4247#discussion_r1731711169, I think we can't just add these CHECKs and the line number issue I was noting is already a problem. There's more going on here so I'm poking around.

jonmeow commented 1 month ago

FYI, #4252 with a different approach on the line numbers, which looks like it fixes the current issues.

dwblaikie commented 1 month ago

FYI, #4252 with a different approach on the line numbers, which looks like it fixes the current issues.

Works for me - I'll abandon/delete this change. Thanks!

dwblaikie commented 1 month ago

@dwblaikie Can you please fix your commit message to not @ me? I will already get notifications from PRs, I don't need notifications from individual commits too. i.e., please fix: dwblaikie@8134dfb

I would generally encourage using PR comments to @ people rather than commits, since the PR is where discussion will occur.

Ah, yeah - sorry, old habit from LLVM where commit message -> description in Phabricator I guess, without the extra layer of commits nested within a review. Will try to avoid writing review descriptions in commits in the future.

jonmeow commented 1 month ago

Ah, yeah - sorry, old habit from LLVM where commit message -> description in Phabricator I guess, without the extra layer of commits nested within a review. Will try to avoid writing review descriptions in commits in the future.

Thanks! Other aspect of this with GitHub, I would've also gotten a notification when the commit was merged. I really wish GH gave me a way of narrowly disabling that.