JeffersonLab / analyzer

HallA C++ Analyzer
BSD 3-Clause "New" or "Revised" License
7 stars 54 forks source link

Pipelining fix1 #152

Closed rwmichaels closed 6 years ago

rwmichaels commented 6 years ago

Fixes that address the original issue #151 (data_type_def not necessarily defined) and the related discussion about why data_type_def was static (should not be), and about the need to pass the data buffer as a constant reference in SplitBuffer. All pretty easy to fix, but it took me awhile to test it because my test stand had been upgraded to CODA 3.07, which opened up a whole other can of worms.

hansenjo commented 6 years ago

Thanks Bob, but I am getting very confused now. It looks like there are three definitions of data_type_def now:

1) member variable of PipeliningModule 2) member variable of Fadc250Module, which inherits from PipeliningModule, therefore masking the member variable in PipeliningModule 3) static variable in PipeliningModule::SplitBuffer, which again masks the member variable

Item (3) was the original problem, but it's still there. I think this is an oversight, and that definition should just be deleted.

Since data_type_def is used both in PipeliningModule::SplitBuffer and Fadc250Module::DecodeOneWord, you have to decide: is this the same data, or different data?

If the same, delete the member variable definition in Fadc250Module. If different, data_type_def needs to be renamed to something else in either PipeliningModule or Fadc250Module.

Could you please check what's appropriate here, make the changes, and submit a new pull request?

Sorry for being a pest, but the way it is now, I think it's even worse than before ....

Thanks, Ole

rwmichaels commented 6 years ago

Sorry, I must have messed that up. I'll take a look.

Bob

----- Original Message ----- From: "Ole Hansen" notifications@github.com To: "JeffersonLab/analyzer" analyzer@noreply.github.com Cc: "Robert Michaels" rom@jlab.org, "Author" author@noreply.github.com Sent: Tuesday, March 13, 2018 12:05:29 PM Subject: Re: [JeffersonLab/analyzer] Pipelining fix1 (#152)

Thanks Bob, but I am getting very confused now. It looks like there are three definitions of now:

1) member variable of PipeliningModule 2) member variable of Fadc250Module, which inherits from PipeliningModule, therefore masking the member variable in PipeliningModule 3) static variable in PipeliningModule::SplitBuffer, which again masks the member variable

3) was the original problem, but it's still there.

I think 3) is an oversight, and that definition should just be deleted.

Since data_type_def is used both in PipeliningModule::SplitBuffer and Fadc250Module::DecodeOneWord, you have to decide: is this the same data, or different data?

If the same, delete the member variable definition in Fadc250Module. If different, data_type_def needs to be renamed in either PipeliningModule or Fadc250Module.

Could you please check what's appropriate here, make the changes, and submit a new pull request?

Sorry for being a pest, but the way it is now, I think it's even worse than before ....

Thanks, Ole

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_JeffersonLab_analyzer_pull_152-23issuecomment-2D372720119&d=DwICaQ&c=lz9TcOasaINaaC3U7FbMev2lsutwpI4--09aP8Lu18s&r=18qXn_9GgntmnnlhbPFXjg&m=vBCyGq2RNZ9yrKn5AH_oDF8sISZ-UpmBbS1qGzXh2Z4&s=Sjn4rQReAb4Gbo6wgrTAUHeayLxaP0OxUyjNZPnG2TM&e=

rwmichaels commented 6 years ago

I fixed it on the same branch, did a pull request and merged. Err, I think.

Bob

----- Original Message ----- From: "Robert Michaels" rom@jlab.org To: "JeffersonLab" reply@reply.github.com Cc: "JeffersonLab/analyzer" analyzer@noreply.github.com, "Author" author@noreply.github.com Sent: Tuesday, March 13, 2018 2:58:51 PM Subject: Re: [JeffersonLab/analyzer] Pipelining fix1 (#152)

Sorry, I must have messed that up. I'll take a look.

Bob

----- Original Message ----- From: "Ole Hansen" notifications@github.com To: "JeffersonLab/analyzer" analyzer@noreply.github.com Cc: "Robert Michaels" rom@jlab.org, "Author" author@noreply.github.com Sent: Tuesday, March 13, 2018 12:05:29 PM Subject: Re: [JeffersonLab/analyzer] Pipelining fix1 (#152)

Thanks Bob, but I am getting very confused now. It looks like there are three definitions of now:

1) member variable of PipeliningModule 2) member variable of Fadc250Module, which inherits from PipeliningModule, therefore masking the member variable in PipeliningModule 3) static variable in PipeliningModule::SplitBuffer, which again masks the member variable

3) was the original problem, but it's still there.

I think 3) is an oversight, and that definition should just be deleted.

Since data_type_def is used both in PipeliningModule::SplitBuffer and Fadc250Module::DecodeOneWord, you have to decide: is this the same data, or different data?

If the same, delete the member variable definition in Fadc250Module. If different, data_type_def needs to be renamed in either PipeliningModule or Fadc250Module.

Could you please check what's appropriate here, make the changes, and submit a new pull request?

Sorry for being a pest, but the way it is now, I think it's even worse than before ....

Thanks, Ole

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_JeffersonLab_analyzer_pull_152-23issuecomment-2D372720119&d=DwICaQ&c=lz9TcOasaINaaC3U7FbMev2lsutwpI4--09aP8Lu18s&r=18qXn_9GgntmnnlhbPFXjg&m=vBCyGq2RNZ9yrKn5AH_oDF8sISZ-UpmBbS1qGzXh2Z4&s=Sjn4rQReAb4Gbo6wgrTAUHeayLxaP0OxUyjNZPnG2TM&e=

hansenjo commented 6 years ago

If you don't mind, I'll clean that up and re-merge it.

Ole

rwmichaels commented 6 years ago

I don't mind at all. Thank you for your patience.

Bob

----- Original Message ----- From: "Ole Hansen" notifications@github.com To: "JeffersonLab/analyzer" analyzer@noreply.github.com Cc: "Robert Michaels" rom@jlab.org, "State change" state_change@noreply.github.com Sent: Tuesday, March 13, 2018 4:06:07 PM Subject: Re: [JeffersonLab/analyzer] Pipelining fix1 (#152)

If you don't mind, I'll clean that up and re-merge it.

Ole

-- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_JeffersonLab_analyzer_pull_152-23issuecomment-2D372800557&d=DwICaQ&c=lz9TcOasaINaaC3U7FbMev2lsutwpI4--09aP8Lu18s&r=18qXn_9GgntmnnlhbPFXjg&m=naidb_IgvOSzylDBIXcTE2E-3hHSHys1AaYtjv1WT54&s=iwR3WbAKjUVNKeEMcMqlGUV-Fqmio61iWbJHJ5rngVE&e=